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

✨ Add command and client for clusterctl alpha rollout restart #3838

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
63 changes: 63 additions & 0 deletions cmd/clusterctl/client/alpha/client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
Copyright 2020 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package alpha

// Client is the alpha client
type Client interface {
Rollout() Rollout
}

// alphaClient implements Client.
type alphaClient struct {
rollout Rollout
}

// ensure alphaClient implements Client.
var _ Client = &alphaClient{}

// Option is a configuration option supplied to New
type Option func(*alphaClient)

// InjectRollout allows to override the rollout implementation to use.
func InjectRollout(rollout Rollout) Option {
return func(c *alphaClient) {
c.rollout = rollout
}
}

// New returns a Client.
func New(options ...Option) Client {
return newAlphaClient(options...)
}

func newAlphaClient(options ...Option) *alphaClient {
client := &alphaClient{}
for _, o := range options {
o(client)
}

// if there is an injected rollout, use it, otherwise use a default one
if client.rollout == nil {
client.rollout = newRolloutClient()
}

return client
}

func (c *alphaClient) Rollout() Rollout {
return c.rollout
}
35 changes: 35 additions & 0 deletions cmd/clusterctl/client/alpha/rollout.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
Copyright 2020 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package alpha

import (
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster"
"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util"
)

// Rollout defines the behavior of a rollout implementation.
type Rollout interface {
ObjectRestarter(cluster.Proxy, util.ResourceTuple, string) error
Copy link
Member

Choose a reason for hiding this comment

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

Why is the expected method called ObjectRestarter instead of just Rollout?

Copy link
Member

Choose a reason for hiding this comment

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

Can we pass a context in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the kubectl naming conventions. There will be other funcs for status, history, pause.
Is the context a must? The internal function does not currently need/use it.

}

var _ Rollout = &rollout{}

type rollout struct{}

func newRolloutClient() Rollout {
return &rollout{}
}
97 changes: 97 additions & 0 deletions cmd/clusterctl/client/alpha/rollout_restarter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
Copyright 2020 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package alpha

import (
"context"
"fmt"
"time"

"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/types"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster"
"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var validResourceTypes = []string{"machinedeployment"}

// ObjectRestarter will issue a restart on the specified cluster-api resource.
func (r *rollout) ObjectRestarter(proxy cluster.Proxy, tuple util.ResourceTuple, namespace string) error {
Arvinderpal marked this conversation as resolved.
Show resolved Hide resolved
switch tuple.Resource {
case "machinedeployment":
Copy link
Member

Choose a reason for hiding this comment

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

Continuing from the question above, given that this only covers MachineDeployments right now, could we have instead RolloutMachineDeployment if we want to be very specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will hopefully be support for kubeadmcontrolplane soon. We will have a case statement for that as well.
Just following kubectl convention in terms of code structure. They also make use of switch statements for different objecs like Deployment, StatefulSet, ...

deployment, err := getMachineDeployment(proxy, tuple.Name, namespace)
if err != nil || deployment == nil {
return errors.Wrapf(err, "failed to fetch %v/%v", tuple.Resource, tuple.Name)
}
if deployment.Spec.Paused {
return errors.Errorf("can't restart paused machinedeployment (run rollout resume first): %v/%v\n", tuple.Resource, tuple.Name)
}
if err := setRestartedAtAnnotation(proxy, tuple.Name, namespace); err != nil {
return err
}
default:
return errors.Errorf("Invalid resource type %v. Valid values: %v", tuple.Resource, validResourceTypes)
}
return nil
}

// getMachineDeployment retrieves the MachineDeployment object corresponding to the name and namespace specified.
func getMachineDeployment(proxy cluster.Proxy, name, namespace string) (*clusterv1.MachineDeployment, error) {
mdObj := &clusterv1.MachineDeployment{}
c, err := proxy.NewClient()
if err != nil {
return nil, err
}
mdObjKey := client.ObjectKey{
Namespace: namespace,
Name: name,
}
if err := c.Get(context.TODO(), mdObjKey, mdObj); err != nil {
return nil, errors.Wrapf(err, "error reading %q %s/%s",
mdObj.GroupVersionKind(), mdObjKey.Namespace, mdObjKey.Name)
}
return mdObj, nil
}

// setRestartedAtAnnotation sets the restartedAt annotation in the MachineDeployment's spec.template.objectmeta.
func setRestartedAtAnnotation(proxy cluster.Proxy, name, namespace string) error {
patch := client.RawPatch(types.MergePatchType, []byte(fmt.Sprintf("{\"spec\":{\"template\":{\"metadata\":{\"annotations\":{\"cluster.x-k8s.io/restartedAt\":\"%v\"}}}}}", time.Now().Format(time.RFC3339))))
Arvinderpal marked this conversation as resolved.
Show resolved Hide resolved
return patchMachineDeployemt(proxy, name, namespace, patch)
}

// patchMachineDeployemt applies a patch to a machinedeployment
func patchMachineDeployemt(proxy cluster.Proxy, name, namespace string, patch client.Patch) error {
cFrom, err := proxy.NewClient()
if err != nil {
return err
}
mdObj := &clusterv1.MachineDeployment{}
mdObjKey := client.ObjectKey{
Namespace: namespace,
Name: name,
}
if err := cFrom.Get(context.TODO(), mdObjKey, mdObj); err != nil {
return errors.Wrapf(err, "error reading %s/%s", mdObj.GetNamespace(), mdObj.GetName())
}

if err := cFrom.Patch(context.TODO(), mdObj, patch); err != nil {
return errors.Wrapf(err, "error while patching %s/%s", mdObj.GetNamespace(), mdObj.GetName())
}
return nil
}
122 changes: 122 additions & 0 deletions cmd/clusterctl/client/alpha/rollout_restarter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
Copyright 2020 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package alpha

import (
"context"
"testing"

. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/test"
"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func Test_ObjectRestarter(t *testing.T) {
type fields struct {
objs []client.Object
tuple util.ResourceTuple
namespace string
}
tests := []struct {
name string
fields fields
wantErr bool
wantAnnotation bool
}{
{
name: "machinedeployment should have restart annotation",
fields: fields{
objs: []client.Object{
&clusterv1.MachineDeployment{
TypeMeta: metav1.TypeMeta{
Kind: "MachineDeployment",
APIVersion: "cluster.x-k8s.io/v1alpha4",
},
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "md-1",
},
},
},
tuple: util.ResourceTuple{
Resource: "machinedeployment",
Name: "md-1",
},
namespace: "default",
},
wantErr: false,
wantAnnotation: true,
},
{
name: "paused machinedeployment should not have restart annotation",
fields: fields{
objs: []client.Object{
&clusterv1.MachineDeployment{
TypeMeta: metav1.TypeMeta{
Kind: "MachineDeployment",
APIVersion: "cluster.x-k8s.io/v1alpha4",
},
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "md-1",
},
Spec: clusterv1.MachineDeploymentSpec{
Paused: true,
},
},
},
tuple: util.ResourceTuple{
Resource: "machinedeployment",
Name: "md-1",
},
namespace: "default",
},
wantErr: true,
wantAnnotation: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
r := newRolloutClient()
proxy := test.NewFakeProxy().WithObjs(tt.fields.objs...)
err := r.ObjectRestarter(proxy, tt.fields.tuple, tt.fields.namespace)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
return
}
g.Expect(err).ToNot(HaveOccurred())
for _, obj := range tt.fields.objs {
cl, err := proxy.NewClient()
g.Expect(err).ToNot(HaveOccurred())
key := client.ObjectKeyFromObject(obj)
md := &clusterv1.MachineDeployment{}
err = cl.Get(context.TODO(), key, md)
g.Expect(err).ToNot(HaveOccurred())
if tt.wantAnnotation {
g.Expect(md.Spec.Template.Annotations).To(HaveKey("cluster.x-k8s.io/restartedAt"))
} else {
g.Expect(md.Spec.Template.Annotations).ToNot(HaveKey("cluster.x-k8s.io/restartedAt"))
}

}
})
}
}
17 changes: 17 additions & 0 deletions cmd/clusterctl/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package client

import (
clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/alpha"
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster"
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/config"
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository"
Expand Down Expand Up @@ -65,6 +66,15 @@ type Client interface {
// ProcessYAML provides a direct way to process a yaml and inspect its
// variables.
ProcessYAML(options ProcessYAMLOptions) (YamlPrinter, error)

// Interface for alpha features in clusterctl
AlphaClient
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joelanford @alexeldeib Can you help me make sense of why this is flagged as an "incompatible change" with go-apidiff? I also tried removing the embedded interface and just putting the func directly in Client interface, but similar issue.

*** Running go-apidiff ***

sigs.k8s.io/cluster-api/cmd/clusterctl/client
  Incompatible changes:
  - AlphaClient.RolloutRestart: added
  Compatible changes:
  - (*clusterctlClient).RolloutRestart: added
  - AlphaClient: added
  - RolloutRestartOptions: added

sigs.k8s.io/cluster-api/cmd/clusterctl/client/rollout
  Compatible changes:
  - ObjectRestarter: added

sigs.k8s.io/cluster-api/cmd/clusterctl/cmd/rollout
  Compatible changes:
  - NewCmdRolloutRestart: added
+ EXIT_VALUE=1
+ set +o xtrace

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/3838/pull-cluster-api-apidiff-main/1319649725397340160/build-log.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

The test is more advisory than required -- it's okay that it fails for PRs to be merged some times. The PR does make all of the changes mentioned, but they all seem reasonable to me (actually a bit surprised the first one is incompatible and the second one is compatible, would have expected those to be flipped since adding to an existing interface would require implementers to update their code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. Unless others see any problems with what I'm doing, I'll just ignore this for now.

}

// AlphaClient exposes the alpha features in clusterctl high-level client library.
type AlphaClient interface {
// RolloutRestart provides rollout restart of cluster-api resources
RolloutRestart(options RolloutRestartOptions) error
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that as other "alpha" commands get added in, this interface could grow large. Since it seems that there are many rollout related functions, we can perhaps embed or add a Rollout rollout.Client interface here.

Something similar to cmd/clusterctl/client/repository/client.go and cmd/clusterctl/client/cluster/client.go.

This way all the rollout related behavior can be kept in that interface and that can be just referenced here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored the code. PTAL

Copy link
Member

Choose a reason for hiding this comment

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

I think @wfernandes was suggesting to actually embed the Rollout interface instead?

Something like:

Suggested change
RolloutRestart(options RolloutRestartOptions) error
type AlphaClient interface {
Rollout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how that would work? Rollout looks like this:

type Rollout interface {
	ObjectRestarter(cluster.Proxy, util.ResourceTuple, string) error
}

Where would the RolloutRestart(options RolloutRestartOptions) error reside?

I believe I had addressed @wfernandes comments with a refactor earlier.

}

// YamlPrinter exposes methods that prints the processed template and
Expand All @@ -82,6 +92,7 @@ type clusterctlClient struct {
configClient config.Client
repositoryClientFactory RepositoryClientFactory
clusterClientFactory ClusterClientFactory
alphaClient alpha.Client
}

// RepositoryClientFactoryInput represents the inputs required by the
Expand Down Expand Up @@ -160,6 +171,12 @@ func newClusterctlClient(path string, options ...Option) (*clusterctlClient, err
client.clusterClientFactory = defaultClusterFactory(client.configClient)
}

// if there is an injected alphaClient, use it, otherwise use a default one.
if client.alphaClient == nil {
Arvinderpal marked this conversation as resolved.
Show resolved Hide resolved
c := alpha.New()
client.alphaClient = c
}

return client, nil
}

Expand Down
4 changes: 4 additions & 0 deletions cmd/clusterctl/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ func (f fakeClient) ProcessYAML(options ProcessYAMLOptions) (YamlPrinter, error)
return f.internalClient.ProcessYAML(options)
}

func (f fakeClient) RolloutRestart(options RolloutRestartOptions) error {
return f.internalClient.RolloutRestart(options)
}

// newFakeClient returns a clusterctl client that allows to execute tests on a set of fake config, fake repositories and fake clusters.
// you can use WithCluster and WithRepository to prepare for the test case.
func newFakeClient(configClient config.Client) *fakeClient {
Expand Down
Loading