Populate machineset status #180

k4leung4 · 2018-05-16T18:48:39Z

This PR was split off from PR #165 to make it a smaller PR.
There are edge cases that will be implemented in a separate PR.

@kubernetes/kube-deploy-reviewers

k8s-ci-robot · 2018-05-16T18:48:52Z

Hi @k4leung4. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs 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.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k4leung4 · 2018-05-16T18:48:54Z

/assign @jessicaochen

jessicaochen · 2018-05-17T17:18:00Z

pkg/controller/machineset/controller.go

 	listers "sigs.k8s.io/cluster-api/pkg/client/listers_generated/cluster/v1alpha1"
 	"sigs.k8s.io/cluster-api/pkg/controller/sharedinformers"
 )

+const (


Perhaps this const belongs in the status file?

jessicaochen · 2018-05-17T17:21:52Z

pkg/controller/machineset/status.go

+			return updatedMS, nil
+		}
+		// Stop retrying if we exceed statusUpdateRetries - the machineSet will be requeued with a rate limit.
+		if i >= statusUpdateRetries {


Do you have a retry test? ie. first time fails, second time works?

spew · 2018-05-17T20:11:10Z

pkg/controller/machineset/controller.go

+	// Always updates status as machines come up or die.
+	_, err = updateMachineSetStatus(c.clusterAPIClient.ClusterV1alpha1().MachineSets(machineSet.Namespace), machineSet, newStatus)
+	if err != nil {
+		return fmt.Errorf("failed to update machine set status, %v", err)


recommend replacing the "," with a ":" as it signifies the error is the reason for why we "failed to update". Also it matches the general style of errors in this project.

spew · 2018-05-17T20:13:42Z

pkg/controller/machineset/controller.go

-	return c.syncReplicas(machineSet, filteredMachines)
+	syncErr := c.syncReplicas(machineSet, filteredMachines)
+
+	ms := machineSet.DeepCopy()


Two questions:

Why is it OK for the code below to continue if syncErr contains an error?

Given The clife_ prefix to the repo name is no longer required #1 above, we can reach line 85 with syncErr populated with an error value. Given In the case of an error on line 85, will cause us to reach line 87 why is it OK that we could potentially lose an error value in "syncErr".

The reason to continue is ensure the MachineSet Status that contains the number of replicas/labeled/ready/available is correct.
syncReplicas may have changed one of the counts in the status and it is worthwhile to update it.

You make a good point about possibly dropping the syncErr.
I changed it such that the two errors are combined.

spew · 2018-05-17T20:40:17Z

pkg/controller/machineset/status.go

+			return updatedMS, nil
+		}
+		// Stop retrying if we exceed statusUpdateRetries - the machineSet will be requeued with a rate limit.
+		if i >= statusUpdateRetries {


Can this package depend on cluster-api/util? If so using the retry out of there would be simpler.

It can depend on cluster-api/util, though I'm not sure it makes it simpler.
The Retry there is more geared towards synchronization of external dependencies, where the exponential backoff makes sense.
I simply want to retry a request in case the server fails for whatever reason.

This PR was split off from PR kubernetes-sigs#165 to make it a smaller PR. There are edge cases that will be implemented in a separate PR.

k4leung4 · 2018-05-17T22:06:58Z

/assign @spew @jessicaochen

jessicaochen

/approve
lgtm (leaving backslash for secondary reviewer)

k8s-ci-robot · 2018-05-18T16:30:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jessicaochen, k4leung4

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~~ [jessicaochen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jessicaochen · 2018-05-18T16:31:09Z

/ok-to-test

k4leung4 · 2018-05-21T15:04:59Z

@spew did you have any other questions or concerns about this PR

k4leung4 · 2018-05-21T16:20:11Z

/cc @spew

spew · 2018-05-21T17:08:34Z

no other comments! lgtm.

spew · 2018-05-21T17:10:30Z

/lgtm

…pdate-to-patch chore: switch from Update() to Patch()

…tes-sigs#180) * Add support to inject custom trusted certs into the machines Resolves kubernetes-sigs#165 Change-Id: I2f399143419553b120e28b46719e7514990fdab0 * update trustedCerts.md fix grammar

…ency-openshift-4.15-ose-cluster-api OCPBUGS-19109: Updating ose-cluster-api images to be consistent with ART

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2018

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 16, 2018

k8s-ci-robot assigned jessicaochen May 16, 2018

k4leung4 force-pushed the ms1 branch 2 times, most recently from ee39a39 to b2ad52a Compare May 16, 2018 20:48

jessicaochen suggested changes May 17, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 17, 2018

spew reviewed May 17, 2018

View reviewed changes

Populate machineset status

4cbc809

This PR was split off from PR kubernetes-sigs#165 to make it a smaller PR. There are edge cases that will be implemented in a separate PR.

k4leung4 force-pushed the ms1 branch from 8feaeb9 to 4cbc809 Compare May 17, 2018 22:06

k8s-ci-robot assigned spew May 17, 2018

jessicaochen approved these changes May 18, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 18, 2018

k8s-ci-robot requested a review from spew May 21, 2018 16:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2018

k8s-ci-robot merged commit cd39d32 into kubernetes-sigs:master May 21, 2018

This was referenced May 21, 2018

MachineSet controller sets MachineSetStatus #54

Closed

Implement machine set status #165

Closed

k4leung4 mentioned this pull request Jul 11, 2018

Define health check strategy for MachineSet #47

Closed

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#180 from newrelic-forks/dennison/u…

0a3606c

…pdate-to-patch chore: switch from Update() to Patch()

fxierh pushed a commit to fxierh/cluster-api that referenced this pull request Sep 14, 2024

Merge pull request kubernetes-sigs#180 from openshift-bot/art-consist…

4e5f522

…ency-openshift-4.15-ose-cluster-api OCPBUGS-19109: Updating ose-cluster-api images to be consistent with ART

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate machineset status #180

Populate machineset status #180

k4leung4 commented May 16, 2018

k8s-ci-robot commented May 16, 2018

k4leung4 commented May 16, 2018

jessicaochen May 17, 2018

k4leung4 May 17, 2018

jessicaochen May 17, 2018

k4leung4 May 17, 2018

spew May 17, 2018

k4leung4 May 17, 2018

spew May 17, 2018

k4leung4 May 17, 2018

spew May 17, 2018

k4leung4 May 17, 2018

k4leung4 commented May 17, 2018

jessicaochen left a comment

k8s-ci-robot commented May 18, 2018

jessicaochen commented May 18, 2018

k4leung4 commented May 21, 2018

k4leung4 commented May 21, 2018

spew commented May 21, 2018

spew commented May 21, 2018

Populate machineset status #180

Populate machineset status #180

Conversation

k4leung4 commented May 16, 2018

k8s-ci-robot commented May 16, 2018

k4leung4 commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k4leung4 commented May 17, 2018

jessicaochen left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 18, 2018

jessicaochen commented May 18, 2018

k4leung4 commented May 21, 2018

k4leung4 commented May 21, 2018

spew commented May 21, 2018

spew commented May 21, 2018