Fixes the infinite reconcile loop#748
Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom May 1, 2019
Merged
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
detiber
reviewed
Apr 30, 2019
Contributor
There was a problem hiding this comment.
What would cause m.Machine to be nil? Wouldn't we always have a Machine?
Contributor
Author
There was a problem hiding this comment.
heh, I think this snuck in. I'll remove it and cluster nil check.
Contributor
There was a problem hiding this comment.
Is there a case where s.Cluster would be nil? Or is this more cautionary?
Contributor
There was a problem hiding this comment.
Instead of doing a Get here, should we be storing the old value when we instantiate the scope instead?
Contributor
Author
There was a problem hiding this comment.
could do, what's the benefit of storing it when we start? seems like that would only increase the amount of time something could update it.
Contributor
There was a problem hiding this comment.
I was thinking that it would ensure that we are only attempting to patch fields that we explicitly changed during the reconciliation. This would avoid us stomping a non-conflicting change that was made by a separate process. For example if another process modified annotations, labels, etc we wouldn't clobber those changes.
Contributor
Author
There was a problem hiding this comment.
So the assertion is that cluster-api will only be managing cluster-api owned fields/labels/annotations/etc and external tools will never touch the cluster-api owned fields/labels/annotations/etc. Conversely cluster-api will never update non-cluster-api owned fields/labels/annotations/etc. If these are correct then I agree with you and this should be storing an object copy at the init of scope.
Contributor
There was a problem hiding this comment.
So, if you consider things like Upgrade, where we are orchestration some type of end state programmatically or end users attempting to modify fields then we definitely have to take in to account external changes.
Contributor
There was a problem hiding this comment.
I would expect the changes mentioned above to be picked up during the next subsequent reconcile and not overwritten by the patch we are doing as part of the current reconciliation
Contributor
Author
There was a problem hiding this comment.
Ok, I think I'm on board. It still feels racy, but it's better than fetching right here, I agree.
Contributor
There was a problem hiding this comment.
Instead of doing a Get here, should we be storing the old value when we instantiate the scope instead?
* Uses patch for updating the cluster and machine specs
- patch does not cause a re-reconcile in the capi controller
* Uses update for updating the cluster and machine status
- update for status is ok since it does not update any of the metadata
no re-reconcile is necessary for the capi controller
Signed-off-by: Chuck Ha <chuckh@vmware.com>
Contributor
Author
|
it feels so good to not get stuck on Running Instance anymore. @detiber ptal! |
Contributor
|
/lgtm |
k8s-ci-robot
added
the
lgtm
detiber
pushed a commit
to detiber/cluster-api-provider-aws
that referenced
this pull request
May 2, 2019
* Uses patch for updating the cluster and machine specs
- patch does not cause a re-reconcile in the capi controller
* Uses update for updating the cluster and machine status
- update for status is ok since it does not update any of the metadata
no re-reconcile is necessary for the capi controller
Signed-off-by: Chuck Ha <chuckh@vmware.com>
k8s-ci-robot
pushed a commit
that referenced
this pull request
May 2, 2019
* Update the releasing docs (#689) * Add error reason to output if fail to checkout an account from boskos (#698) * Temporary workaround a data issue in boskos service (#699) * Update checkout_account.py to not reuse connections (#700) * Fix checkout_account.py (#702) * Make hack/checkin_account.py executable (#703) * Fix: all traffic ingress rule triggers fatal nil dereference (#697) * fix: respect all traffic security group rules (and others) For anything besides tcp, udp, icmp, and icmpv6 there is no applicable notion of "port range." AWS omits FromPort and ToPort in its responses, causing a fatal nil dereference when attempting to read any security groups with e.g. an "all traffic" rule. * fix: omit description when empty string * fix: handle more security groups without crashing This commit cleans up and clarifies a few of the less obvious components of the previous work. * fix: handle more security groups without crashing Address linter failures. * fix: handle more security groups without crashing Usage needs to match declaration. Computers are sticklers about that sort of thing. * fix: handle more security groups without crashing Add clarifying comment to serializer function. * Fixes a bug and adds tests for kubeadm defaults (#707) The pointers were not working as expected so the API is changing to be more functional and leverage kubernetes' DeepCopy function. * Update listed v1.14 AMIs to v1.14.1 (#708) * Update listed v1.14 AMIs to v1.14.1 * Update README with list of published AMIs/Kubernetes versions * GZIP user-data (#710) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Make sure Calico can talk IP-in-IP (#701) * MAke sure Calico can talk IP-in-IP * Add IP in IP protocol to the control plane security group * Add IPv4 protocol definition and make sure it's handled properly. * Make port ranges AWS complient and security groups more restrictive. * Fix security groups * Adds tests to kubeadm defaults (#709) Attempt at documenting the assumptions made in the kubeadm defaults code. Signed-off-by: Chuck Ha <chuckh@vmware.com> * Logging (#713) * Adds logr as dependency Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use logr in the cluster actuator This only creates the logger. Does not yet swap out actual klog calls. Signed-off-by: Chuck Ha <chuckh@vmware.com> * update bazel Signed-off-by: Chuck Ha <chuckh@vmware.com> * update Signed-off-by: Chuck Ha <chuckh@vmware.com> * Switch dep to use release-0.1 branch instead of version (#715) * Adds logr as dependency (#714) Adds context for logs and removes excessive logging Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure `make manifests` generates machines file for HA control plane too. (#720) * Add HA machines template * Introduce HA machines file in `make manifests` target * Add clusterawsadm as make dependency to manifests make target. (#721) Ensures manifests are generated from the current state of the source. Assuming $GOPATH/bin is in the $PATH * Update to Go 1.12 (#719) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Add ability to override Organization ID for image lookups (#723) * Add ability to override Organization ID for image lookups * Update pkg/cloud/aws/services/ec2/ami.go Co-Authored-By: detiber <detiberusj@vmware.com> * Add updated generated crd * feat: support customizing root device size (#718) * feat: support customizing root device size * chore: re-generate CRDs * fix: update formatting * chore: add comment describing Service.sdkToInstance * chore: make service.SDKToInstance public * Rename BUILD -> BUILD.bazel for consistency (#724) find . -type file -name BUILD -not -path "./vendor/*" | xargs -n1 -I{} -- git mv {} {}.bazel Preferred build name changed in 3788fb1 Fixes #722 * Adds retry-on-conflict during updates (#725) * Adds retry-on-conflict during updates Signed-off-by: Chuck Ha <chuckh@vmware.com> * adds note about status update caveat Signed-off-by: Chuck Ha <chuckh@vmware.com> * clarify errors/comments Signed-off-by: Chuck Ha <chuckh@vmware.com> * Add the HA machines configuration to bazel (#733) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure bazel is the correct version (#731) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update OWNERS_ALIASES and SECURITY_CONTACTS (#712) * Fix the prow jobs (#735) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Fix markdown formatting (#736) * extract fmt from release tool (#738) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use DEFAULT_REGION as the default and REGION as the supplied (#739) Signed-off-by: Chuck Ha <chuckh@vmware.com> * e2e testing improvement (#743) * Bump kind version * Remove docker load in favor of kind load for e2e cluster Signed-off-by: Chuck Ha <chuckh@vmware.com> * fix: Don't try to update root size when it's unset (#726) * fix: Don't try to update root size when it's unset This commit looks for empty RootDeviceSize in the spec and ignores it. Otherwise, none of our control plane machines were updating with this error: ``` E0418 23:07:48.250925 1 controller.go:214] Error updating machine "ns/controlplane-2": found attempt to change immutable state for machine "controlplane-2": ["Root volume size cannot be mutated from 8 to 0"] ``` * fix: updates without specifying a root volume size Add unit test. * fix: updates without specifying a root volume size Fix gofmt. * Scope nodeRef to workload cluster (#744) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fix NPE on delete bastion host (#746) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Documentation for creating a new cluster on a different AWS account (#728) * Initial draft of documentation for Cluster creation using cross account role assumption * Update roleassumption.md Complete the document. * cleanup the documentation for roleassumption * Resolved the comments: role assumption documentation. * Fix minor issues - roleassumption.md * resolve more comments to roleassumption.md * Resolve more comments - roleassumption.md * include machines-ha.yaml.template in release artifacts (#741) * Update AWS sdk, improve log in machine actuator delete (#747) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fixes the infinite reconcile loop (#748) * Uses patch for updating the cluster and machine specs - patch does not cause a re-reconcile in the capi controller * Uses update for updating the cluster and machine status - update for status is ok since it does not update any of the metadata no re-reconcile is necessary for the capi controller Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update Gopkg.lock and cleanup Makefile (#751) * Update cluster-api release-0.1 vendor (#750) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Reduce the number of re-reconciles (#752) Signed-off-by: Chuck Ha <chuckh@vmware.com>
richardchen-db
pushed a commit
to databricks/cluster-api-provider-aws-1
that referenced
this pull request
Jan 14, 2023
* Update the releasing docs (kubernetes-sigs#689) * Add error reason to output if fail to checkout an account from boskos (kubernetes-sigs#698) * Temporary workaround a data issue in boskos service (kubernetes-sigs#699) * Update checkout_account.py to not reuse connections (kubernetes-sigs#700) * Fix checkout_account.py (kubernetes-sigs#702) * Make hack/checkin_account.py executable (kubernetes-sigs#703) * Fix: all traffic ingress rule triggers fatal nil dereference (kubernetes-sigs#697) * fix: respect all traffic security group rules (and others) For anything besides tcp, udp, icmp, and icmpv6 there is no applicable notion of "port range." AWS omits FromPort and ToPort in its responses, causing a fatal nil dereference when attempting to read any security groups with e.g. an "all traffic" rule. * fix: omit description when empty string * fix: handle more security groups without crashing This commit cleans up and clarifies a few of the less obvious components of the previous work. * fix: handle more security groups without crashing Address linter failures. * fix: handle more security groups without crashing Usage needs to match declaration. Computers are sticklers about that sort of thing. * fix: handle more security groups without crashing Add clarifying comment to serializer function. * Fixes a bug and adds tests for kubeadm defaults (kubernetes-sigs#707) The pointers were not working as expected so the API is changing to be more functional and leverage kubernetes' DeepCopy function. * Update listed v1.14 AMIs to v1.14.1 (kubernetes-sigs#708) * Update listed v1.14 AMIs to v1.14.1 * Update README with list of published AMIs/Kubernetes versions * GZIP user-data (kubernetes-sigs#710) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Make sure Calico can talk IP-in-IP (kubernetes-sigs#701) * MAke sure Calico can talk IP-in-IP * Add IP in IP protocol to the control plane security group * Add IPv4 protocol definition and make sure it's handled properly. * Make port ranges AWS complient and security groups more restrictive. * Fix security groups * Adds tests to kubeadm defaults (kubernetes-sigs#709) Attempt at documenting the assumptions made in the kubeadm defaults code. Signed-off-by: Chuck Ha <chuckh@vmware.com> * Logging (kubernetes-sigs#713) * Adds logr as dependency Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use logr in the cluster actuator This only creates the logger. Does not yet swap out actual klog calls. Signed-off-by: Chuck Ha <chuckh@vmware.com> * update bazel Signed-off-by: Chuck Ha <chuckh@vmware.com> * update Signed-off-by: Chuck Ha <chuckh@vmware.com> * Switch dep to use release-0.1 branch instead of version (kubernetes-sigs#715) * Adds logr as dependency (kubernetes-sigs#714) Adds context for logs and removes excessive logging Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure `make manifests` generates machines file for HA control plane too. (kubernetes-sigs#720) * Add HA machines template * Introduce HA machines file in `make manifests` target * Add clusterawsadm as make dependency to manifests make target. (kubernetes-sigs#721) Ensures manifests are generated from the current state of the source. Assuming $GOPATH/bin is in the $PATH * Update to Go 1.12 (kubernetes-sigs#719) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Add ability to override Organization ID for image lookups (kubernetes-sigs#723) * Add ability to override Organization ID for image lookups * Update pkg/cloud/aws/services/ec2/ami.go Co-Authored-By: detiber <detiberusj@vmware.com> * Add updated generated crd * feat: support customizing root device size (kubernetes-sigs#718) * feat: support customizing root device size * chore: re-generate CRDs * fix: update formatting * chore: add comment describing Service.sdkToInstance * chore: make service.SDKToInstance public * Rename BUILD -> BUILD.bazel for consistency (kubernetes-sigs#724) find . -type file -name BUILD -not -path "./vendor/*" | xargs -n1 -I{} -- git mv {} {}.bazel Preferred build name changed in 3788fb1 Fixes kubernetes-sigs#722 * Adds retry-on-conflict during updates (kubernetes-sigs#725) * Adds retry-on-conflict during updates Signed-off-by: Chuck Ha <chuckh@vmware.com> * adds note about status update caveat Signed-off-by: Chuck Ha <chuckh@vmware.com> * clarify errors/comments Signed-off-by: Chuck Ha <chuckh@vmware.com> * Add the HA machines configuration to bazel (kubernetes-sigs#733) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Ensure bazel is the correct version (kubernetes-sigs#731) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update OWNERS_ALIASES and SECURITY_CONTACTS (kubernetes-sigs#712) * Fix the prow jobs (kubernetes-sigs#735) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Fix markdown formatting (kubernetes-sigs#736) * extract fmt from release tool (kubernetes-sigs#738) Signed-off-by: Chuck Ha <chuckh@vmware.com> * Use DEFAULT_REGION as the default and REGION as the supplied (kubernetes-sigs#739) Signed-off-by: Chuck Ha <chuckh@vmware.com> * e2e testing improvement (kubernetes-sigs#743) * Bump kind version * Remove docker load in favor of kind load for e2e cluster Signed-off-by: Chuck Ha <chuckh@vmware.com> * fix: Don't try to update root size when it's unset (kubernetes-sigs#726) * fix: Don't try to update root size when it's unset This commit looks for empty RootDeviceSize in the spec and ignores it. Otherwise, none of our control plane machines were updating with this error: ``` E0418 23:07:48.250925 1 controller.go:214] Error updating machine "ns/controlplane-2": found attempt to change immutable state for machine "controlplane-2": ["Root volume size cannot be mutated from 8 to 0"] ``` * fix: updates without specifying a root volume size Add unit test. * fix: updates without specifying a root volume size Fix gofmt. * Scope nodeRef to workload cluster (kubernetes-sigs#744) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fix NPE on delete bastion host (kubernetes-sigs#746) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Documentation for creating a new cluster on a different AWS account (kubernetes-sigs#728) * Initial draft of documentation for Cluster creation using cross account role assumption * Update roleassumption.md Complete the document. * cleanup the documentation for roleassumption * Resolved the comments: role assumption documentation. * Fix minor issues - roleassumption.md * resolve more comments to roleassumption.md * Resolve more comments - roleassumption.md * include machines-ha.yaml.template in release artifacts (kubernetes-sigs#741) * Update AWS sdk, improve log in machine actuator delete (kubernetes-sigs#747) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Fixes the infinite reconcile loop (kubernetes-sigs#748) * Uses patch for updating the cluster and machine specs - patch does not cause a re-reconcile in the capi controller * Uses update for updating the cluster and machine status - update for status is ok since it does not update any of the metadata no re-reconcile is necessary for the capi controller Signed-off-by: Chuck Ha <chuckh@vmware.com> * Update Gopkg.lock and cleanup Makefile (kubernetes-sigs#751) * Update cluster-api release-0.1 vendor (kubernetes-sigs#750) Signed-off-by: Vince Prignano <vincepri@vmware.com> * Reduce the number of re-reconciles (kubernetes-sigs#752) Signed-off-by: Chuck Ha <chuckh@vmware.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
no re-reconcile is necessary for the capi controller
Signed-off-by: Chuck Ha chuckh@vmware.com
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):apparently there is no issue, but creating a cluster was weird.
Special notes for your reviewer:
I believe this will be the first step to fixing the long-create times we were seeing.
Release note: