Skip to content
Closed
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
25 changes: 23 additions & 2 deletions pkg/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,14 @@ func LoadClientset() (*kubernetes.Clientset, error) {
return kubernetes.NewForConfig(cfg)
}

func IsStatusAvailable(client runtimeclient.Client, name string) bool {
func WaitForStatusAvailable(client runtimeclient.Client, name string) bool {
key := types.NamespacedName{
Namespace: MachineAPINamespace,
Name: name,
}
clusterOperator := &configv1.ClusterOperator{}

if err := wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, 10*time.Minute, func() (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit description says waits for 3 minutes, which is WaitMedium, but it's actually waiting for 10 minutes, should we update one of these? I think 3 minutes to become available should be sufficient shouldn't it?

Suggested change
if err := wait.PollImmediate(RetryShort, 10*time.Minute, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, WaitMedium, func() (bool, error) {

Copy link
Member Author

Choose a reason for hiding this comment

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

3 min is the minimum time the mao expects the pod to have been available https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/sync.go#L28

5 min is the total the mao waits for the pod to rollout out https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/sync.go#L30

10 min is where we set the bar in our tests to consider this a failure. I set 10 to account for scenarios where this is running in parallel with tests that are disrupting the status.

I'm ok to put any lower (>5m) if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, that makes sense, happy to leave as 10 mins now commit description is updated

if err := client.Get(context.TODO(), key, clusterOperator); err != nil {
klog.Errorf("error querying api for OperatorStatus object: %v, retrying...", err)
return false, nil
Expand All @@ -152,6 +152,27 @@ func IsStatusAvailable(client runtimeclient.Client, name string) bool {
return true
}

func IsStatusDegraded(client runtimeclient.Client, name string) (bool, error) {
key := types.NamespacedName{
Namespace: MachineAPINamespace,
Name: name,
}
clusterOperator := &configv1.ClusterOperator{}
if err := client.Get(context.TODO(), key, clusterOperator); err != nil {
return false, fmt.Errorf("error querying api for OperatorStatus object: %v, retrying...", err)
}

isAvailable := cov1helpers.IsStatusConditionTrue(clusterOperator.Status.Conditions, configv1.OperatorAvailable)
isDegraded := cov1helpers.IsStatusConditionTrue(clusterOperator.Status.Conditions, configv1.OperatorDegraded)
isProgressing := cov1helpers.IsStatusConditionTrue(clusterOperator.Status.Conditions, configv1.OperatorProgressing)

klog.Infof("Condition: %q is %v, expected true", configv1.OperatorAvailable, isAvailable)
klog.Infof("Condition: %q is %v, expected true", configv1.OperatorDegraded, isDegraded)
klog.Infof("Condition: %q is %v, expected false", configv1.OperatorProgressing, isProgressing)

return isAvailable && isDegraded && !isProgressing, nil
}

func WaitForValidatingWebhook(client runtimeclient.Client, name string) bool {
key := types.NamespacedName{Name: name}
webhook := &admissionregistrationv1beta1.ValidatingWebhookConfiguration{}
Expand Down
2 changes: 1 addition & 1 deletion pkg/operators/cluster-autoscaler-operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,6 @@ var _ = Describe("[Feature:Operators] Cluster autoscaler cluster operator status
var err error
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())
Expect(framework.IsStatusAvailable(client, "cluster-autoscaler")).To(BeTrue())
Expect(framework.WaitForStatusAvailable(client, "cluster-autoscaler")).To(BeTrue())
})
})
2 changes: 1 addition & 1 deletion pkg/operators/cluster-machine-approver.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@ var _ = Describe("[Feature:Operators] Cluster Machine Approver Cluster Operator
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

Expect(framework.IsStatusAvailable(client, cmaClusterOperator)).To(BeTrue())
Expect(framework.WaitForStatusAvailable(client, cmaClusterOperator)).To(BeTrue())
})
})
47 changes: 46 additions & 1 deletion pkg/operators/machine-api-operator.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
package operators

import (
"context"
"fmt"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/openshift/cluster-api-actuator-pkg/pkg/framework"
v1 "k8s.io/api/core/v1"
"k8s.io/klog"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var (
Expand Down Expand Up @@ -73,6 +77,47 @@ var _ = Describe("[Feature:Operators] Machine API cluster operator status should
It("be available", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())
Expect(framework.IsStatusAvailable(client, "machine-api")).To(BeTrue())
Expect(framework.WaitForStatusAvailable(client, "machine-api")).To(BeTrue())
})

It("be degraded when a pod owned by the operator is prevented from being available", func() {
c, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

// https://github.com/openshift/machine-api-operator/blob/d234cceb5de18b83aa0609d17db7d835f2d78973/pkg/operator/sync.go#L310-L313
mAPIControllersLabels := map[string]string{
"api": "clusterapi",
"k8s-app": "controller",
}

Eventually(func() (bool, error) {
// get machine API controllers pods
podList := &v1.PodList{}
if err := c.List(context.TODO(), podList, client.MatchingLabels(mAPIControllersLabels)); err != nil {
klog.Errorf("failed to list pods: %v", err)
return false, nil
}
if len(podList.Items) < 1 {
klog.Errorf("list of pods is empty")
return false, nil
}
Comment on lines +100 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this matter? If the list of pods is empty, status should be degraded right?

Choose a reason for hiding this comment

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

I think it doesn't matter, but do we expect pods not to be running when at this part of the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well my thought was we go through this cycle, delete a pod, status isn't degraded yet, so start again, pod has not been recreated yet, could the status be degraded by this point? I am not sure, I would assume they are independent since they're different controllers controlling these actions


// delete machine API controllers pods
for k := range podList.Items {
if err := c.Delete(context.Background(), &podList.Items[k]); err != nil {
klog.Errorf("failed to delete pod: %v", err)
return false, nil
}
}

isStatusDegraded, err := framework.IsStatusDegraded(c, "machine-api")
if err != nil {
return false, nil
}

return isStatusDegraded, nil
}, framework.WaitLong, framework.RetryShort).Should(BeTrue())

Expect(framework.WaitForStatusAvailable(c, "machine-api")).To(BeTrue())
})
})