search

kubernetes-retired / cluster-api-bootstrap-provider-kubeadm

LEGACY REPO. NEW CODE IS https://github.com/kubernetes-sigs/cluster-api/tree/master/bootstrap/kubeadm
Apache License 2.0
62 stars 67 forks source link

Patch kubeadmconfig objects only if no error occurs during reconciliation #242

Closed accepting closed 4 years ago

accepting commented 4 years ago

Signed-off-by: JunLi lijun.git@gmail.com

What this PR does / why we need it: There is no need to update KubeadmConfig objects if any error occurs during the reconciliation.

k8s-ci-robot commented 4 years ago

Hi @accepting. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
chuckha commented 4 years ago

/ok-to-test

chuckha commented 4 years ago

@accepting This is right. We never set the config value except in the successful case so there is no reason to issue a patch in an error case, but I'm curious why you want this change? Was there a bug you were encountering that this fixes?

accepting commented 4 years ago

@chuckha In our use cases, the bootstrap objects support update. When we try to update a bootstrap object, we found the object was updated even if some error occurs afterwards.

chuckha commented 4 years ago

That's interesting. Does an update to the bootstrap object actually modify the cluster?

accepting commented 4 years ago

Yes, with the help of kubeadm phases, we update the control planes/worker nodes' settings by updating their bootstrap objects.

chuckha commented 4 years ago

I'm fine merging this and reverting or changing behavior back if we need to start populating the error message.

/approve

k8s-ci-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: accepting, chuckha

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/blob/master/OWNERS)~~ [chuckha] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
detiber commented 4 years ago

/lgtm