Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Compute ControlPlane version for a managed topology #5059

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions controllers/topology/desired_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"k8s.io/apiserver/pkg/storage/names"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
"sigs.k8s.io/cluster-api/controllers/external"
"sigs.k8s.io/cluster-api/util/version"
)

// computeDesiredState computes the desired state of the cluster topology.
Expand Down Expand Up @@ -136,15 +137,81 @@ func computeControlPlane(class *clusterTopologyClass, current *clusterTopologySt
}
}

// Computes the desired Kubernetes version for the control plane.
controlPlaneVersion, err := computeControlPlaneVersion(current)
if err != nil {
return nil, errors.Wrap(err, "failed to compute spec.version for the ControlPlane object")
}

// Sets the desired Kubernetes version for the control plane.
// TODO: improve this logic by adding support for version upgrade component by component
if err := unstructured.SetNestedField(controlPlane.UnstructuredContent(), current.cluster.Spec.Topology.Version, "spec", "version"); err != nil {
if err := unstructured.SetNestedField(controlPlane.UnstructuredContent(), controlPlaneVersion, "spec", "version"); err != nil {
return nil, errors.Wrap(err, "failed to set spec.version in the ControlPlane object")
}

return controlPlane, nil
}

// computeControlPlaneVersion computes the ControlPlane version based on the current clusterTopologyState.
// TODO: we also have to handle the following cases:
// * ControlPlane.spec.version != ControlPlane.status.version, i.e. ControlPLane rollout is already in progress.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably handle this now within this function, is there any reason we're holding?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked to @fabriziopandini and we thought it might be good idea to keep it easy for now as there are all kinds of things that can go wrong when the ClusterClass version is changed again during a rollout (either when ControlPlane is not yet rolled out completely and also if at least a subset of MachineDeployments are not rolled out yet).

Only adding the case in l.156 should be relatively simple.

Copy link
Member Author

@sbueringer sbueringer Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the case from l.156.

If we also want to add l.157 I would prefer waiting for the MachineDeployment compute and reconcile PRs to be merged first. Then I would also implement computeMachineDeploymentVersions in this PR. This should make it easier to reason about the overall cluster state during upgrades and all edge cases.

// * ControlPlane.spec.version != MachineDeployment[].spec.template.spec.version, i.e. one of the MachineDeployments has
// a different version then the ControlPlane.
func computeControlPlaneVersion(current *clusterTopologyState) (string, error) {
currentTopologyVersion := current.cluster.Spec.Topology.Version

// ControlPlane does not exist yet, use currentTopologyVersion for new ControlPlane.
if current.controlPlane.object == nil {
return currentTopologyVersion, nil
}

currentControlPlaneVersion, err := getControlPlaneVersion(current.controlPlane.object)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
currentControlPlaneVersion, err := getControlPlaneVersion(current.controlPlane.object)
version, err := getControlPlaneVersion(current.controlPlane.object)

Copy link
Member Author

@sbueringer sbueringer Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped the current prefix everywhere. Now I have:

  • topologyVersion
  • controlPlaneVersion
  • controlPlaneStatusVersion

I think in this func the prefixes are useful to keep the logic as easily readable as possible.

If you prefer the following seems like a good alternative to me:

  • topologyVersion
  • version
  • statusVersion

I guess the missing prefix in combination with the func name should provide enough context to infer version is the controlPlaneVersion.

WDYT?

if err != nil {
return "", err
}

// ControlPlane already has the currentTopologyVersion.
if currentControlPlaneVersion == currentTopologyVersion {
return currentTopologyVersion, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return is the exact same as the last one, can we simplify and merge them?

Copy link
Member Author

@sbueringer sbueringer Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that. I thought it might be easier to read and understand if I spell out the different cases explicitly (especially as there will be a few more).

I combined those two and added a comment which contains both cases.


// ControlPlane downgrade is not allowed.
if err := detectDowngrade(currentControlPlaneVersion, currentTopologyVersion); err != nil {
return "", err
}

// ControlPlane will be upgraded.
return currentTopologyVersion, nil
}

// getControlPlaneVersion gets the .spec.version of a ControlPlane.
func getControlPlaneVersion(controlPlane *unstructured.Unstructured) (string, error) {
controlPlaneVersion, ok, err := unstructured.NestedString(controlPlane.UnstructuredContent(), "spec", "version")
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return "", errors.Wrap(err, "failed to get spec.version from ControlPlane")
}
if !ok {
return "", errors.New("failed to get spec.version from ControlPlane: not found")
}
return controlPlaneVersion, nil
}

// detectDowngrade compares currentVersion and desiredVersion and returns an error if they either cannot be parsed
// or if a downgrade is detected.
func detectDowngrade(currentVersion, desiredVersion string) error {
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
currentVersionParsed, err := version.ParseMajorMinorPatch(currentVersion)
if err != nil {
return errors.Errorf("failed to parse current version %q: %v", currentVersion, err)
}
desiredVersionParsed, err := version.ParseMajorMinorPatch(desiredVersion)
if err != nil {
return errors.Errorf("failed to parse desired version %q: %v", desiredVersion, err)
}
if desiredVersionParsed.LT(currentVersionParsed) {
return errors.Errorf("downgrade from %s to %s is not supported", currentVersion, desiredVersion)
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}

// computeCluster computes the desired state for the Cluster object.
// NOTE: Some fields of the Cluster’s fields contribute to defining how a Cluster should look like (e.g. Cluster.Spec.Topology),
// while some other fields should be managed as part of the actual Cluster (e.g. Cluster.Spec.ControlPlaneRef); in this func
Expand Down
80 changes: 80 additions & 0 deletions controllers/topology/desired_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,86 @@ func TestComputeControlPlane(t *testing.T) {
})
}

func TestComputeControlPlaneVersion(t *testing.T) {
tests := []struct {
name string
topologyVersion string
currentControlPlane *unstructured.Unstructured
want string
wantErr bool
}{
{
name: "ControlPlane does not exist",
topologyVersion: "v1.21.0",
currentControlPlane: nil,
want: "v1.21.0",
},
{
name: "ControlPlane does exist, but without version",
topologyVersion: "v1.21.0",
currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").Obj(),
wantErr: true,
},
{
name: "ControlPlane does exist, with same version (no-op)",
topologyVersion: "v1.21.0",
currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").WithVersion("v1.21.0").Obj(),
want: "v1.21.0",
},
{
name: "ControlPlane does exist, with newer version (downgrade)",
topologyVersion: "v1.21.0",
currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").WithVersion("v1.22.0").Obj(),
wantErr: true,
},
{
name: "ControlPlane does exist, with invalid version",
topologyVersion: "v1.21.0",
currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").WithVersion("invalid-version").Obj(),
wantErr: true,
},
{
name: "ControlPlane does exist, with valid version but invalid topology version",
topologyVersion: "invalid-version",
currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").WithVersion("v1.21.0").Obj(),
wantErr: true,
},
{
name: "ControlPlane does exist, with older version (upgrade)",
topologyVersion: "v1.21.0",
currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").WithVersion("v1.20.0").Obj(),
want: "v1.21.0",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

current := &clusterTopologyState{
cluster: &clusterv1.Cluster{
Spec: clusterv1.ClusterSpec{
Topology: &clusterv1.Topology{
Version: tt.topologyVersion,
},
},
},
controlPlane: controlPlaneTopologyState{
object: tt.currentControlPlane,
},
}

got, err := computeControlPlaneVersion(current)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
} else {
g.Expect(err).NotTo(HaveOccurred())
}
g.Expect(got).To(Equal(tt.want))
})
}
}

func TestComputeCluster(t *testing.T) {
// generated objects
infrastructureCluster := newFakeInfrastructureCluster(metav1.NamespaceDefault, "infrastructureCluster1").Obj()
Expand Down
12 changes: 12 additions & 0 deletions controllers/topology/object_builders_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ type fakeControlPlane struct {
namespace string
name string
infrastructureMachineTemplate *unstructured.Unstructured
version *string
}

func newFakeControlPlane(namespace, name string) *fakeControlPlane {
Expand All @@ -409,6 +410,11 @@ func (f *fakeControlPlane) WithInfrastructureMachineTemplate(t *unstructured.Uns
return f
}

func (f *fakeControlPlane) WithVersion(version string) *fakeControlPlane {
f.version = &version
return f
}

func (f *fakeControlPlane) Obj() *unstructured.Unstructured {
obj := &unstructured.Unstructured{}
obj.SetAPIVersion(fakeControlPlaneProviderGroupVersion.String())
Expand All @@ -428,5 +434,11 @@ func (f *fakeControlPlane) Obj() *unstructured.Unstructured {
panic(err)
}
}

if f.version != nil {
if err := unstructured.SetNestedField(obj.UnstructuredContent(), *f.version, "spec", "version"); err != nil {
panic(err)
}
}
return obj
}