[release-4.1] Bug 1706082: Add Spec.Configuration to MachineConfigPool, render controller writes it

[cgwalters]

Cherry-picked from 37e8466

See #765 (comment)

MachineConfigPool needs a Spec.Configuration and Status.Configuration
[just like other objects][1] so that we can properly detect state.
Currently there's a race because the render controller may set Status.Configuration
while the pool's Status still has Updated, so one can't reliably check whether the
pool is at a given config.

With this, ownership is clear: the render controller sets the spec, and the node controller
updates the status.

[1] https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status)

[openshift-ci-robot]

@cgwalters: This pull request references an invalid Bugzilla bug:

• expected the bug to target the "4.1.z" release, but it targets "4.2.0" instead

In response to this:

Bug 1706082: Add Spec.Configuration to MachineConfigPool, render controller writes it

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.

[runcom]

This patch makes sure the Generation field for an MCP is correctly increased (it's not right now in 4.1 because we never update the Spec field of the MCP).
With this, syncRequiredMachineConfigPools correctly checks that the MCP is actually rolling the new upgrade changes.

The race we've been seeing is:

• 18:07:56: new Operator goes up
• start syncing
• syncRequiredMachineConfigPools passes cause pool master is idle but didn't start updating to new rendered-mc
• 18:09:28: new version reported
• 18:10:06: the master pool starts rolling the new templates

[cgwalters]

Going to wait for a CI run before I force push again to retrigger Prow's check of the Bugzilla target (which is now 4.1.z again)

[cgwalters]

/bugzilla refresh

[openshift-ci-robot]

@cgwalters: This pull request references a valid Bugzilla bug.

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.

[runcom]

Looks like there is a compilation issue in e2e

[cgwalters]

Oh duh, looking at the diff the test suite changes are entangled with some prep PRs like 9970f24
which itself is built on top of #610 which touches the whole codebase. Going to try redoing the e2e test changes without that last one.

[cgwalters]

ci/prow/e2e-aws-op — Job succeeded.

🎉

[runcom]

/approve

[kikisdeliveryservice]

yay!!

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, kikisdeliveryservice, runcom

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 [cgwalters,kikisdeliveryservice,runcom]

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

[cgwalters]

Anyone know who takes care of "Pending — Not mergeable. Needs cherry-pick-approved label. " ?

[kikisdeliveryservice]

Anyone know who takes care of "Pending — Not mergeable. Needs cherry-pick-approved label. " ?

yeah i talked to @runcom and i'll get that on next week.

[eparis]

Parent BZ is still not verified by QA. Reaching out to QA might be helpful when I re-review for 4.1.4 inclusion tomorrow.

I see you have some CI testing around this. But I'm not entirely sure of the source of these changes. Is this a combination of #765 and #773 ?

What's the impact of not merging this?

[runcom]

What's the impact of not merging this?

we won't tell exactly that an upgrade completed because we'll report "OK" doing the comparison with the old version (this is what this fixes)

[cgwalters]

But I'm not entirely sure of the source of these changes. Is this a combination of #765 and #773 ?

Yep, exactly.

What's the impact of not merging this?

The bug symptoms would persist; the MCO could claim it was done upgrading when it hadn't (transiently).

And/or we'd have to come up with a different fix which...seems at best equally risky. We know this code works in 4.2.

[eparis]

we've been in master since May 16. Even though QA has not VERIFIED I'm going to let this through. We likely have had a reasonable about of CI soak time in master.