Bug 1947684: delay kubelet config readiness until after pools and controller config are ready by harche · Pull Request #2517 · openshift/machine-config-operator

harche · 2021-04-08T14:15:17Z

Signed-off-by: Harshal Patil harpatil@redhat.com

- What I did

- How to verify it

- Description for the changelog

rphillips · 2021-04-09T13:20:53Z

/retitle Bug 1947684: delay kubelet config readiness until after pools and controller config are ready
/retest

openshift-ci-robot · 2021-04-09T13:22:11Z

@harche: This pull request references Bugzilla bug 1947684, which is invalid:

expected the bug to target the "4.8.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1947684: delay kubelet config readiness until after pools and controller config are ready

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.

rphillips · 2021-04-09T13:22:47Z

/bugzilla refresh

openshift-ci-robot · 2021-04-09T13:22:54Z

@rphillips: This pull request references Bugzilla bug 1947684, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (schoudha@redhat.com), skipping review request.

Details

In response to this:

/bugzilla refresh

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.

rphillips · 2021-04-09T13:25:03Z

/lgtm

rphillips · 2021-04-09T13:27:19Z

/lgtm

sinnykumari

LGTM
Test needs to pass first and would like to see e2e-gcp-op passes.

rphillips · 2021-04-09T14:36:23Z

@harche Can you add this to the PR:

diff --git a/pkg/controller/kubelet-config/kubelet_config_controller_test.go b/pkg/controller/kubelet-config/kubelet_config_controller_test.go
index f5c0c960..2bb29378 100644
--- a/pkg/controller/kubelet-config/kubelet_config_controller_test.go
+++ b/pkg/controller/kubelet-config/kubelet_config_controller_test.go
@@ -11,6 +11,7 @@ import (
        osev1 "github.com/openshift/api/config/v1"
        oseconfigfake "github.com/openshift/client-go/config/clientset/versioned/fake"
        oseinformersv1 "github.com/openshift/client-go/config/informers/externalversions"
+       corev1 "k8s.io/api/core/v1"
        "k8s.io/apimachinery/pkg/api/equality"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        "k8s.io/apimachinery/pkg/runtime"
@@ -111,6 +112,25 @@ func newControllerConfig(name string, platform osev1.PlatformType) *mcfgv1.Contr
                                },
                        },
                },
+               Status: mcfgv1.ControllerConfigStatus{
+                       Conditions: []mcfgv1.ControllerConfigStatusCondition{
+                               {
+                                       Type:    mcfgv1.TemplateControllerCompleted,
+                                       Status:  corev1.ConditionTrue,
+                                       Message: "",
+                               },
+                               {
+                                       Type:    mcfgv1.TemplateControllerRunning,
+                                       Status:  corev1.ConditionFalse,
+                                       Message: "",
+                               },
+                               {
+                                       Type:    mcfgv1.TemplateControllerFailing,
+                                       Status:  corev1.ConditionFalse,
+                                       Message: "",
+                               },
+                       },
+               },
        }
        return cc
 }

rphillips · 2021-04-09T15:23:23Z

/lgtm

yuqi-zhang

Why are we gating the kubeletconfig on controllerconfig, since I don't think they depend on each other at all? Is this basically trying to wait until the initial configuration for a cluster has settled before applying master kubeletconfigs? In that case wouldn't this cause 1 extra reboot for masters during install, because the kubeletconfig sometimes doesn't get applied until initial configuration is generated, which is somewhat counter to what we want? (maybe I'm mis-understanding something here)

I feel like for the bug we should fix it in a way such that the bootstrap already handles kubeletconfigs, such that the initial pool configuration has the kubeletconfig in place.

yuqi-zhang · 2021-04-09T16:58:14Z

Could you update the commit message such that you mention why this is being bumped

The kubelet config is dependent on the controller config here: https://github.com/harche/machine-config-operator/blob/f28b2c2a4102228af4b6754522e13384711969e7/pkg/controller/kubelet-config/kubelet_config_controller.go#L321-L341

Ideally, initial rollout would be nice, but that isn't how the feature works today. Changes to kubelet configurations require a reboot.

Ah my bad. I missed that connection. I think this code does make sense to make the wait explicit. that said, I'm not sure if this will achieve the desired effect of waiting for the initial configuration of the pool to settle (the linked bugzilla), since

machine-config-operator/pkg/controller/kubelet-config/kubelet_config_controller.go

Line 478 in 5527f84

return ctrl.syncStatusOnly(cfg, err, "could not get kubelet config key: %v", err)

was blocking it before now anyways. The race between the KCC generating MCs from kubeletconfigs and the in-cluster MCC generating the initial node configuration still exists I think.

If we want to go this route, maybe

machine-config-operator/pkg/controller/kubelet-config/kubelet_config_controller.go

Line 466 in 5527f84

if pool.Spec.Configuration.Name == "" {

we should instead check if the pool has a rendered config set to the pool's spec. Then the kubeletconfig should always get generated after an initial rendered configs gets generated (we want that, since it won't contain kubeletconfigs like bootstrap), and thus bypass the "not found" issue.

Alternatively, I still think it might be a cleaner solution just to add a bootstrap reader mode to the kubeletconfigcontoller and have the bootstrap MCC call it (achieving day 1), which if we want to do in the future, we'd have to revert our proposed fix here,

Ideally, we need to update the code to run at bootstrap. Perhaps @QiWang19 can look into updating the KubeletConfig as a second pass. I would like to unblock Omer.

The node_controller does something similar to see if the pool is ready...
https://github.com/harche/machine-config-operator/blob/5527f842eda1fd51538590a592d3c95a3cd679d9/pkg/controller/node/node_controller.go#L720-L727

Signed-off-by: Ryan Phillips <rphillip@redhat.com>

rphillips · 2021-04-12T12:40:11Z

/retest

yuqi-zhang

Approving since this seems to fix the bootstrap race condition for kubeletconfigs.

sinnykumari · 2021-04-13T12:24:28Z

/lgtm
/retest

openshift-ci-robot · 2021-04-13T12:24:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harche, rphillips, sinnykumari, yuqi-zhang

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sinnykumari,yuqi-zhang]

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

openshift-ci · 2021-04-13T15:14:51Z

@harche: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/okd-e2e-aws	`5527f84`	link	`/test okd-e2e-aws`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

openshift-bot · 2021-04-13T15:15:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-04-13T17:51:45Z

@harche: All pull requests linked via external trackers have merged:

openshift/machine-config-operator#2517

Bugzilla bug 1947684 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1947684: delay kubelet config readiness until after pools and controller config are ready

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.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2021

openshift-ci-robot requested review from sinnykumari and yuqi-zhang April 8, 2021 14:17

harche force-pushed the bump_maxtries branch from 97a69e6 to b340baa Compare April 9, 2021 11:32

openshift-ci-robot changed the title ~~WIP: bump up maxRetries to 18 in kubelet config controller~~ Bug 1947684: delay kubelet config readiness until after pools and controller config are ready Apr 9, 2021

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 9, 2021

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 9, 2021

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Apr 9, 2021

openshift-ci-robot assigned rphillips Apr 9, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

harche force-pushed the bump_maxtries branch from b340baa to cafb2f7 Compare April 9, 2021 13:25

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

sinnykumari reviewed Apr 9, 2021

View reviewed changes

rphillips force-pushed the bump_maxtries branch from cafb2f7 to f28b2c2 Compare April 9, 2021 15:14

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

yuqi-zhang requested changes Apr 9, 2021

View reviewed changes

Wait for controller config before applying kubeletconfig

5527f84

Signed-off-by: Ryan Phillips <rphillip@redhat.com>

rphillips force-pushed the bump_maxtries branch from f28b2c2 to 5527f84 Compare April 9, 2021 19:23

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

yuqi-zhang approved these changes Apr 13, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2021

omertuc mentioned this pull request Apr 13, 2021

Single node reserve 3GiB of system memory to avoid alerts openshift/release#17403

Merged

openshift-ci-robot assigned sinnykumari Apr 13, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2021

openshift-merge-robot merged commit 3fe1270 into openshift:master Apr 13, 2021

yuqi-zhang mentioned this pull request Oct 5, 2021

[OCPNODE-673] Add kubeletconfig to bootstrap mode #2547

Merged

Conversation

harche commented Apr 8, 2021

Uh oh!

rphillips commented Apr 9, 2021

Uh oh!

openshift-ci-robot commented Apr 9, 2021

Uh oh!

rphillips commented Apr 9, 2021

Uh oh!

openshift-ci-robot commented Apr 9, 2021

Uh oh!

rphillips commented Apr 9, 2021

Uh oh!

rphillips commented Apr 9, 2021

Uh oh!

sinnykumari left a comment

Choose a reason for hiding this comment

Uh oh!

rphillips commented Apr 9, 2021

Uh oh!

rphillips commented Apr 9, 2021

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

yuqi-zhang Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

rphillips Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

yuqi-zhang Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

rphillips Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

rphillips commented Apr 12, 2021

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

sinnykumari commented Apr 13, 2021

Uh oh!

openshift-ci-robot commented Apr 13, 2021

Uh oh!

openshift-ci Bot commented Apr 13, 2021

Uh oh!

openshift-bot commented Apr 13, 2021

Uh oh!

openshift-ci-robot commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants