Skip to content

Set 10 minute timeout on webhook and deployment operations #183

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.

Sign up for GitHub

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

Closed
Danil-Grigorev wants to merge 2 commits into from
Closed

Set 10 minute timeout on webhook and deployment operations #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e93e6f
Set 10 minute timeout on webhook operations
Aug 6, 2020
fef754e
Add framework support for custom deployment timeouts
Aug 7, 2020
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
38 changes: 21 additions & 17 deletions pkg/framework/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"reflect"
"time"

kappsapi "k8s.io/api/apps/v1"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -12,15 +13,15 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

// GetDeployment gets deployment object by name and namespace.
func GetDeployment(c client.Client, name, namespace string) (*kappsapi.Deployment, error) {
// GetDeployment gets deployment object by name and namespace with custom timeout
func GetDeployment(c client.Client, name, namespace string, timeout time.Duration) (*kappsapi.Deployment, error) {
key := types.NamespacedName{
Namespace: namespace,
Name: name,
}
d := &kappsapi.Deployment{}

if err := wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, timeout, func() (bool, error) {
if err := c.Get(context.TODO(), key, d); err != nil {
klog.Errorf("Error querying api for Deployment object %q: %v, retrying...", name, err)
return false, nil
Expand All @@ -32,9 +33,9 @@ func GetDeployment(c client.Client, name, namespace string) (*kappsapi.Deploymen
return d, nil
}

// DeleteDeployment deletes the specified deployment
func DeleteDeployment(c client.Client, deployment *kappsapi.Deployment) error {
return wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
// DeleteDeployment deletes the specified deployment with custom timeout
func DeleteDeployment(c client.Client, deployment *kappsapi.Deployment, timeout time.Duration) error {
return wait.PollImmediate(RetryShort, timeout, func() (bool, error) {
if err := c.Delete(context.TODO(), deployment); err != nil {
klog.Errorf("error querying api for deployment object %q: %v, retrying...", deployment.Name, err)
return false, nil
Expand All @@ -43,10 +44,11 @@ func DeleteDeployment(c client.Client, deployment *kappsapi.Deployment) error {
})
}

// UpdateDeployment updates the specified deployment
func UpdateDeployment(c client.Client, name, namespace string, updated *kappsapi.Deployment) error {
return wait.PollImmediate(RetryShort, WaitMedium, func() (bool, error) {
Copy link
Member

@enxebre enxebre Aug 7, 2020

Choose a reason for hiding this comment

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

why do we need to wrap this? now UpdateDeployment is unused. Can't we just have a single waitForDeploymentUpdate?

d, err := GetDeployment(c, name, namespace)
// UpdateDeployment updates the specified deployment with custom timeout
func UpdateDeployment(c client.Client, name, namespace string,
updated *kappsapi.Deployment, timeout time.Duration) error {
return wait.PollImmediate(RetryShort, timeout, func() (bool, error) {
d, err := GetDeployment(c, name, namespace, WaitShort)
if err != nil {
klog.Errorf("Error getting deployment: %v", err)
return false, nil
Expand All @@ -59,10 +61,10 @@ func UpdateDeployment(c client.Client, name, namespace string, updated *kappsapi
})
}

// IsDeploymentAvailable returns true if the deployment has one or more availabe replicas
func IsDeploymentAvailable(c client.Client, name, namespace string) bool {
if err := wait.PollImmediate(RetryShort, WaitLong, func() (bool, error) {
d, err := GetDeployment(c, name, namespace)
// IsDeploymentAvailable returns true if the deployment has one or more availabe replicas with custom timeout
func IsDeploymentAvailable(c client.Client, name, namespace string, timeout time.Duration) bool {
if err := wait.PollImmediate(RetryShort, timeout, func() (bool, error) {
d, err := GetDeployment(c, name, namespace, WaitShort)
if err != nil {
klog.Errorf("Error getting deployment: %v", err)
return false, nil
Expand All @@ -82,9 +84,11 @@ func IsDeploymentAvailable(c client.Client, name, namespace string) bool {
return true
}

// IsDeploymentSynced returns true if provided deployment spec matched one found on cluster
func IsDeploymentSynced(c client.Client, dep *kappsapi.Deployment, name, namespace string) bool {
d, err := GetDeployment(c, name, namespace)
// IsDeploymentSynced returns true if provided deployment spec
// matched one found on cluster with custom timeout
func IsDeploymentSynced(c client.Client, dep *kappsapi.Deployment,
name, namespace string, timeout time.Duration) bool {
d, err := GetDeployment(c, name, namespace, timeout)
if err != nil {
klog.Errorf("Error getting deployment: %v", err)
return false
Expand Down
23 changes: 15 additions & 8 deletions pkg/framework/webhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package framework
import (
"context"
"fmt"
"time"

mapiv1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
Expand All @@ -19,12 +20,18 @@ var DefaultValidatingWebhookConfiguration = mapiv1.NewValidatingWebhookConfigura
// DefaultMutatingWebhookConfiguration is a default mutating webhook configuration resource provided by MAO
var DefaultMutatingWebhookConfiguration = mapiv1.NewMutatingWebhookConfiguration()

// Webhooks may take up to 10 minutes to sync. When changed we first have to wait for a full Deployment
// rollout (5 minutes) and DaemonSet rollout (another 5 minutes) to occur before they are synced. Then the
// process starts all over again. This applies to all webhook operations.
// TODO: Make MAO resource state management unblocking
var waitWebhooks = 10 * time.Minute

// GetMutatingWebhookConfiguration gets MutatingWebhookConfiguration object by name
func GetMutatingWebhookConfiguration(c client.Client, name string) (*admissionregistrationv1.MutatingWebhookConfiguration, error) {
key := client.ObjectKey{Name: name}
existing := &admissionregistrationv1.MutatingWebhookConfiguration{}

if err := wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
if err := c.Get(context.TODO(), key, existing); err != nil {
klog.Errorf("Error querying api for MutatingWebhookConfiguration object %q: %v, retrying...", name, err)
return false, nil
Expand All @@ -41,7 +48,7 @@ func GetValidatingWebhookConfiguration(c client.Client, name string) (*admission
key := client.ObjectKey{Name: name}
existing := &admissionregistrationv1.ValidatingWebhookConfiguration{}

if err := wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
if err := c.Get(context.TODO(), key, existing); err != nil {
klog.Errorf("Error querying api for ValidatingWebhookConfiguration object %q: %v, retrying...", name, err)
return false, nil
Expand All @@ -55,7 +62,7 @@ func GetValidatingWebhookConfiguration(c client.Client, name string) (*admission

// DeleteValidatingWebhookConfiguration deletes the specified ValidatingWebhookConfiguration object
func DeleteValidatingWebhookConfiguration(c client.Client, webhookConfiguraiton *admissionregistrationv1.ValidatingWebhookConfiguration) error {
return wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
return wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
if err := c.Delete(context.TODO(), webhookConfiguraiton); apierrors.IsNotFound(err) {
return true, nil
} else if err != nil {
Expand All @@ -68,7 +75,7 @@ func DeleteValidatingWebhookConfiguration(c client.Client, webhookConfiguraiton

// DeleteMutatingWebhookConfiguration deletes the specified MutatingWebhookConfiguration object
func DeleteMutatingWebhookConfiguration(c client.Client, webhookConfiguraiton *admissionregistrationv1.MutatingWebhookConfiguration) error {
return wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
return wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
if err := c.Delete(context.TODO(), webhookConfiguraiton); apierrors.IsNotFound(err) {
return true, nil
} else if err != nil {
Expand All @@ -81,7 +88,7 @@ func DeleteMutatingWebhookConfiguration(c client.Client, webhookConfiguraiton *a

// UpdateMutatingWebhookConfiguration updates the specified mutating webhook configuration
func UpdateMutatingWebhookConfiguration(c client.Client, updated *admissionregistrationv1.MutatingWebhookConfiguration) error {
return wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
return wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
existing, err := GetMutatingWebhookConfiguration(c, updated.Name)
if err != nil {
klog.Errorf("Error getting MutatingWebhookConfiguration: %v", err)
Expand All @@ -97,7 +104,7 @@ func UpdateMutatingWebhookConfiguration(c client.Client, updated *admissionregis

// UpdateValidatingWebhookConfiguration updates the specified mutating webhook configuration
func UpdateValidatingWebhookConfiguration(c client.Client, updated *admissionregistrationv1.ValidatingWebhookConfiguration) error {
return wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
return wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
existing, err := GetValidatingWebhookConfiguration(c, updated.Name)
if err != nil {
klog.Errorf("Error getting ValidatingWebhookConfiguration: %v", err)
Expand All @@ -113,7 +120,7 @@ func UpdateValidatingWebhookConfiguration(c client.Client, updated *admissionreg

// IsMutatingWebhookConfigurationSynced expects a matching MutatingWebhookConfiguration to be present in the cluster
func IsMutatingWebhookConfigurationSynced(c client.Client) bool {
if err := wait.PollImmediate(RetryShort, WaitMedium, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
existing, err := GetMutatingWebhookConfiguration(c, DefaultMutatingWebhookConfiguration.Name)
if err != nil {
klog.Errorf("Error getting MutatingWebhookConfiguration: %v", err)
Expand All @@ -137,7 +144,7 @@ func IsMutatingWebhookConfigurationSynced(c client.Client) bool {

// IsValidatingWebhookConfigurationSynced expects a matching MutatingWebhookConfiguration to be present in the cluster
func IsValidatingWebhookConfigurationSynced(c client.Client) bool {
if err := wait.PollImmediate(RetryShort, WaitMedium, func() (bool, error) {
if err := wait.PollImmediate(RetryShort, waitWebhooks, func() (bool, error) {
existing, err := GetValidatingWebhookConfiguration(c, DefaultValidatingWebhookConfiguration.Name)
if err != nil {
klog.Errorf("Error getting MutatingWebhookConfiguration: %v", err)
Expand Down
3 changes: 2 additions & 1 deletion pkg/infra/spot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ var _ = Describe("[Feature:Machines] Running on Spot", func() {
Expect(client.Create(ctx, deployment)).To(Succeed())
delObjects[deployment.Name] = deployment

Expect(framework.IsDeploymentAvailable(client, deployment.Name, deployment.Namespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, deployment.Name,
deployment.Namespace, framework.WaitLong)).To(BeTrue())
})

var machine *mapiv1.Machine
Expand Down
3 changes: 2 additions & 1 deletion pkg/operators/cluster-autoscaler-operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ var _ = Describe("[Feature:Operators] Cluster autoscaler operator deployment sho
var err error
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())
Expect(framework.IsDeploymentAvailable(client, "cluster-autoscaler-operator", framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, "cluster-autoscaler-operator",
framework.MachineAPINamespace, framework.WaitLong)).To(BeTrue())
})
})

Expand Down
3 changes: 2 additions & 1 deletion pkg/operators/cluster-machine-approver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ var _ = Describe("[Feature:Operators] Cluster Machine Approver deployment", func
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

Expect(framework.IsDeploymentAvailable(client, cmaDeployment, cmaNamespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, cmaDeployment,
cmaNamespace, framework.WaitLong)).To(BeTrue())
})
})

Expand Down
62 changes: 23 additions & 39 deletions pkg/operators/machine-api-operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,107 +2,94 @@ package operators

import (
"fmt"
"time"

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

var (
maoDeployment = "machine-api-operator"
maoManagedDeployment = "machine-api-controllers"
)

// Webhooks may take up to 10 minutes to sync. When changed we first have to wait for a full Deployment
// rollout (5 minutes) and DaemonSet rollout (another 5 minutes) to occur before they are synced.
var defaultReconcileTime = 10 * time.Minute

var _ = Describe("[Feature:Operators] Machine API operator deployment should", func() {
defer GinkgoRecover()
var client client.Client

It("be available", func() {
client, err := framework.LoadClient()
BeforeEach(func() {
var err error
client, err = framework.LoadClient()
Expect(err).NotTo(HaveOccurred())
Expect(framework.IsDeploymentAvailable(client, maoDeployment, framework.MachineAPINamespace)).To(BeTrue())
By(fmt.Sprintf("checking deployment %q is available", maoDeployment))
Expect(framework.IsDeploymentAvailable(client, maoDeployment,
framework.MachineAPINamespace, framework.WaitLong)).To(BeTrue())
})

It("reconcile controllers deployment", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

initialDeployment, err := framework.GetDeployment(client, maoManagedDeployment, framework.MachineAPINamespace)
initialDeployment, err := framework.GetDeployment(client, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)
Expect(err).NotTo(HaveOccurred())

By(fmt.Sprintf("checking deployment %q is available", maoManagedDeployment))
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)).To(BeTrue())

By(fmt.Sprintf("deleting deployment %q", maoManagedDeployment))
Expect(framework.DeleteDeployment(client, initialDeployment)).NotTo(HaveOccurred())
Expect(framework.DeleteDeployment(client, initialDeployment, defaultReconcileTime)).NotTo(HaveOccurred())

By(fmt.Sprintf("checking deployment %q is available again", maoManagedDeployment))
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)).To(BeTrue())

By(fmt.Sprintf("checking deployment %q spec matches", maoManagedDeployment))
Expect(framework.IsDeploymentSynced(client, initialDeployment, maoManagedDeployment, framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentSynced(client, initialDeployment, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)).To(BeTrue())
})

It("maintains deployment spec", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

initialDeployment, err := framework.GetDeployment(client, maoManagedDeployment, framework.MachineAPINamespace)
initialDeployment, err := framework.GetDeployment(client, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)
Expect(err).NotTo(HaveOccurred())

By(fmt.Sprintf("checking deployment %q is available", maoManagedDeployment))
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)).To(BeTrue())

changedDeployment := initialDeployment.DeepCopy()
changedDeployment.Spec.Replicas = pointer.Int32Ptr(0)

By(fmt.Sprintf("updating deployment %q", maoManagedDeployment))
Expect(framework.UpdateDeployment(client, maoManagedDeployment, framework.MachineAPINamespace, changedDeployment)).NotTo(HaveOccurred())
Expect(framework.UpdateDeployment(client, maoManagedDeployment, framework.MachineAPINamespace, changedDeployment, defaultReconcileTime)).NotTo(HaveOccurred())

By(fmt.Sprintf("checking deployment %q spec matches", maoManagedDeployment))
Expect(framework.IsDeploymentSynced(client, initialDeployment, maoManagedDeployment, framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentSynced(client, initialDeployment, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)).To(BeTrue())

By(fmt.Sprintf("checking deployment %q is available again", maoManagedDeployment))
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace)).To(BeTrue())
Expect(framework.IsDeploymentAvailable(client, maoManagedDeployment, framework.MachineAPINamespace, defaultReconcileTime)).To(BeTrue())

})

It("reconcile mutating webhook configuration", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

Expect(framework.IsMutatingWebhookConfigurationSynced(client)).To(BeTrue())
})

It("reconcile validating webhook configuration", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

Expect(framework.IsValidatingWebhookConfigurationSynced(client)).To(BeTrue())
})

It("recover after validating webhook configuration deletion", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

Expect(framework.DeleteValidatingWebhookConfiguration(client, framework.DefaultValidatingWebhookConfiguration)).To(Succeed())

Expect(framework.IsValidatingWebhookConfigurationSynced(client)).To(BeTrue())
})

It("recover after mutating webhook configuration deletion", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

Expect(framework.DeleteMutatingWebhookConfiguration(client, framework.DefaultMutatingWebhookConfiguration)).To(Succeed())

Expect(framework.IsMutatingWebhookConfigurationSynced(client)).To(BeTrue())
})

It("maintains spec after mutating webhook configuration change and preserve caBundle", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

initial, err := framework.GetMutatingWebhookConfiguration(client, framework.DefaultMutatingWebhookConfiguration.Name)
Expect(initial).ToNot(BeNil())
Expect(err).To(Succeed())
Expand All @@ -127,9 +114,6 @@ var _ = Describe("[Feature:Operators] Machine API operator deployment should", f
})

It("maintains spec after validating webhook configuration change and preserve caBundle", func() {
client, err := framework.LoadClient()
Expect(err).NotTo(HaveOccurred())

initial, err := framework.GetValidatingWebhookConfiguration(client, framework.DefaultValidatingWebhookConfiguration.Name)
Expect(initial).ToNot(BeNil())
Expect(err).To(Succeed())
Expand Down