From 3d6902a4f66972e135e168afc648235387b15b5f Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Thu, 10 Aug 2023 21:37:38 -0600 Subject: [PATCH 01/20] (feat): automatically delete temporary resources Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- api/v1/agentconfig_types.go | 14 +++++++++++ controllers/agentaction_controller.go | 35 +++++++++++++++++++++++++-- tests/integration/suite_test.go | 4 +-- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/api/v1/agentconfig_types.go b/api/v1/agentconfig_types.go index ba37c452..891544ab 100644 --- a/api/v1/agentconfig_types.go +++ b/api/v1/agentconfig_types.go @@ -65,6 +65,10 @@ type AgentConfigSpec struct { // +optional VolumeSize string `json:"volumeSize,omitempty" mapstructure:"volumeSize,omitempty"` + // TTLSecondsAfterFinished set the time limit of the lifetime of a Job + // that has finished execution. + TTLSecondsAfterFinished *int32 `json:"ttlSecondsAfterFinished,omitempty" mapstructure:"ttlSecondsAftterFinished,omitempty"` + // PullPolicy specifies when to pull the Porter Agent image. The default // is to use PullAlways when the tag is canary or latest, and PullIfNotPresent // otherwise. @@ -346,6 +350,16 @@ func (c AgentConfigSpecAdapter) ToPorterDocument() ([]byte, error) { return yaml.Marshal(raw) } +// GetTTLSecondsAfterFinished returns the config value of +// TTLSecondsAfterFinished defaults to 600 +func (c AgentConfigSpecAdapter) GetTTLSecondsAfterFinished() *int32 { + if c.original.TTLSecondsAfterFinished == nil { + defaultTTLSeconds := int32(600) + c.original.TTLSecondsAfterFinished = &defaultTTLSeconds + } + return c.original.TTLSecondsAfterFinished +} + // PluginConfigList is the list implementation of the Plugins map. // The list is sorted based on the plugin names alphabetically. type PluginsConfigList struct { diff --git a/controllers/agentaction_controller.go b/controllers/agentaction_controller.go index bacf152a..0d92307a 100644 --- a/controllers/agentaction_controller.go +++ b/controllers/agentaction_controller.go @@ -254,6 +254,16 @@ func (r *AgentActionReconciler) createAgentVolume(ctx context.Context, log logr. GenerateName: action.Name + "-", Namespace: action.Namespace, Labels: labels, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: action.APIVersion, + Kind: action.Kind, + Name: action.Name, + UID: action.UID, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + }, + }, }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, @@ -302,6 +312,16 @@ func (r *AgentActionReconciler) createConfigSecret(ctx context.Context, log logr GenerateName: action.Name + "-", Namespace: action.Namespace, Labels: labels, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: action.APIVersion, + Kind: action.Kind, + Name: action.Name, + UID: action.UID, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + }, + }, }, Type: corev1.SecretTypeOpaque, Immutable: ptr.To(true), @@ -338,6 +358,16 @@ func (r *AgentActionReconciler) createWorkdirSecret(ctx context.Context, log log GenerateName: action.Name + "-", Namespace: action.Namespace, Labels: labels, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: action.APIVersion, + Kind: action.Kind, + Name: action.Name, + UID: action.UID, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + }, + }, }, Type: corev1.SecretTypeOpaque, Immutable: ptr.To(true), @@ -420,8 +450,9 @@ func (r *AgentActionReconciler) createAgentJob(ctx context.Context, log logr.Log }, }, Spec: batchv1.JobSpec{ - Completions: ptr.To(int32(1)), - BackoffLimit: agentCfg.GetRetryLimit(), + Completions: ptr.To(int32(1)), + BackoffLimit: agentCfg.GetRetryLimit(), + TTLSecondsAfterFinished: agentCfg.GetTTLSecondsAfterFinished(), Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ GenerateName: action.Name + "-", diff --git a/tests/integration/suite_test.go b/tests/integration/suite_test.go index 827704a5..969559f1 100644 --- a/tests/integration/suite_test.go +++ b/tests/integration/suite_test.go @@ -16,7 +16,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -48,7 +48,7 @@ var _ = BeforeSuite(func(done Done) { By("bootstrapping test environment") testEnv = &envtest.Environment{ - UseExistingCluster: pointer.Bool(true), + UseExistingCluster: ptr.To(true), CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")}, } From 9e05026cc93c159f68d8c695ea02efe5ca705f0a Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Mon, 14 Aug 2023 14:25:39 -0600 Subject: [PATCH 02/20] (feat): automatically delete temporary resources Co-authored-by: Troy Connor Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- api/v1/agentconfig_types.go | 18 ++++++--------- api/v1/agentconfig_types_test.go | 22 +++++++++++++++++++ api/v1/zz_generated.deepcopy.go | 5 +++++ .../crd/bases/getporter.org_agentconfigs.yaml | 6 +++++ controllers/agentaction_controller_test.go | 2 ++ 5 files changed, 42 insertions(+), 11 deletions(-) diff --git a/api/v1/agentconfig_types.go b/api/v1/agentconfig_types.go index 891544ab..a5683694 100644 --- a/api/v1/agentconfig_types.go +++ b/api/v1/agentconfig_types.go @@ -67,6 +67,7 @@ type AgentConfigSpec struct { // TTLSecondsAfterFinished set the time limit of the lifetime of a Job // that has finished execution. + // +kubebuilder:default:=600 TTLSecondsAfterFinished *int32 `json:"ttlSecondsAfterFinished,omitempty" mapstructure:"ttlSecondsAftterFinished,omitempty"` // PullPolicy specifies when to pull the Porter Agent image. The default @@ -331,11 +332,16 @@ func (c AgentConfigSpecAdapter) GetInstallationServiceAccount() string { return c.original.InstallationServiceAccount } -// SetRetryAnnotation flags the resource to retry its last operation. +// GetRetryLimit returns the config value of RetryLimit func (c *AgentConfigSpecAdapter) GetRetryLimit() *int32 { return c.original.RetryLimit } +// GetTTLSecondsAfterFinished returns the config value of TTLSecondsAfterFinished +func (c *AgentConfigSpecAdapter) GetTTLSecondsAfterFinished() *int32 { + return c.original.TTLSecondsAfterFinished +} + func (c AgentConfigSpecAdapter) ToPorterDocument() ([]byte, error) { raw := struct { SchemaType string `yaml:"schemaType"` @@ -350,16 +356,6 @@ func (c AgentConfigSpecAdapter) ToPorterDocument() ([]byte, error) { return yaml.Marshal(raw) } -// GetTTLSecondsAfterFinished returns the config value of -// TTLSecondsAfterFinished defaults to 600 -func (c AgentConfigSpecAdapter) GetTTLSecondsAfterFinished() *int32 { - if c.original.TTLSecondsAfterFinished == nil { - defaultTTLSeconds := int32(600) - c.original.TTLSecondsAfterFinished = &defaultTTLSeconds - } - return c.original.TTLSecondsAfterFinished -} - // PluginConfigList is the list implementation of the Plugins map. // The list is sorted based on the plugin names alphabetically. type PluginsConfigList struct { diff --git a/api/v1/agentconfig_types_test.go b/api/v1/agentconfig_types_test.go index a67f8924..743b638d 100644 --- a/api/v1/agentconfig_types_test.go +++ b/api/v1/agentconfig_types_test.go @@ -370,6 +370,28 @@ func TestAgentConfigSpecAdapter_GetRetryLimit(t *testing.T) { } } +func TestAgentConfigSpecAdapter_GetTTLSecondsAfterFinished(t *testing.T) { + // var testdataDefault int32 = 600 + var testdatauserSubmitted int32 = 700 + testCases := []struct { + name string + TTLSecondsAfterFinished *int32 + expected *int32 + }{ + // {name: "default", TTLSecondsAfterFinished: nil, expected: &testdataDefault}, + {name: "user submitted", TTLSecondsAfterFinished: &testdatauserSubmitted, expected: &testdatauserSubmitted}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + adapter := NewAgentConfigSpecAdapter(AgentConfigSpec{ + TTLSecondsAfterFinished: tc.TTLSecondsAfterFinished, + }) + result := adapter.GetTTLSecondsAfterFinished() + require.Equal(t, tc.expected, result) + }) + } +} + func TestHashString(t *testing.T) { str := hashString("fake-string") assert.Equal(t, "ab19e45285992b247dd281213f803479", str) diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index c3230ca5..8ec60d2f 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -249,6 +249,11 @@ func (in *AgentConfigList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AgentConfigSpec) DeepCopyInto(out *AgentConfigSpec) { *out = *in + if in.TTLSecondsAfterFinished != nil { + in, out := &in.TTLSecondsAfterFinished, &out.TTLSecondsAfterFinished + *out = new(int32) + **out = **in + } if in.RetryLimit != nil { in, out := &in.RetryLimit, &out.RetryLimit *out = new(int32) diff --git a/config/crd/bases/getporter.org_agentconfigs.yaml b/config/crd/bases/getporter.org_agentconfigs.yaml index 32a7ab58..a42958c8 100644 --- a/config/crd/bases/getporter.org_agentconfigs.yaml +++ b/config/crd/bases/getporter.org_agentconfigs.yaml @@ -103,6 +103,12 @@ spec: Porter will request when running the Porter Agent. It is used to determine what the storage class will be for the volume requested type: string + ttlSecondsAfterFinished: + default: 600 + description: TTLSecondsAfterFinished set the time limit of the lifetime + of a Job that has finished execution. + format: int32 + type: integer volumeSize: description: VolumeSize is the size of the persistent volume that Porter will request when running the Porter Agent. It is used to diff --git a/controllers/agentaction_controller_test.go b/controllers/agentaction_controller_test.go index e7b38d7b..e1256fc8 100644 --- a/controllers/agentaction_controller_test.go +++ b/controllers/agentaction_controller_test.go @@ -405,6 +405,7 @@ func TestAgentActionReconciler_createAgentVolume(t *testing.T) { } spec := porterv1.NewAgentConfigSpecAdapter(agentCfg) pvc, err := controller.createAgentVolume(context.Background(), logr.Discard(), action, spec) + require.NoError(t, err) // Verify the pvc properties @@ -412,6 +413,7 @@ func TestAgentActionReconciler_createAgentVolume(t *testing.T) { assert.Equal(t, "porter-hello-", pvc.GenerateName, "incorrect pvc name") assert.Equal(t, []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, pvc.Spec.AccessModes, "incorrect pvc access modes") assert.Equal(t, pvc.Spec.Resources.Requests[corev1.ResourceStorage], resource.MustParse("128Mi")) + assert.Equal(t, pvc.OwnerReferences[0].Name, action.Name) } else { assert.Equal(t, "existing-", pvc.GenerateName, "incorrect pvc name") assert.Equal(t, []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany}, pvc.Spec.AccessModes, "incorrect pvc access modes") From 896facbfecb8e026cd195dc18f155d17dc56c13d Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Thu, 17 Aug 2023 21:33:40 -0600 Subject: [PATCH 03/20] trying to use controllerutil.SetControllerReference Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 44 +++++++--------------- controllers/agentconfig_controller_test.go | 10 ----- controllers/credentialset_controller.go | 16 +++----- controllers/installation_controller.go | 16 +++----- controllers/parameterset_controller.go | 16 +++----- 5 files changed, 28 insertions(+), 74 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index 804b4a90..a69f0ed7 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -14,7 +14,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -145,6 +144,7 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) if created { log.V(Log4Debug).Info("Created new temporary persistent volume claim.", "name", pvc.Name) } + // Use porter to finish reconciling the agent config err = r.applyAgentConfig(ctx, log, pvc, agentCfg) if err != nil { @@ -208,16 +208,6 @@ func (r *AgentConfigReconciler) createEmptyPluginVolume(ctx context.Context, log Namespace: agentCfg.Namespace, Labels: labels, Annotations: agentCfg.GetPluginsPVCNameAnnotation(), - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: agentCfg.APIVersion, - Kind: agentCfg.Kind, - Name: agentCfg.Name, - UID: agentCfg.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, @@ -231,6 +221,9 @@ func (r *AgentConfigReconciler) createEmptyPluginVolume(ctx context.Context, log if storageClassName != "" { pvc.Spec.StorageClassName = &storageClassName } + if err := controllerutil.SetControllerReference(agentCfg, pvc, r.Scheme); err != nil { + return nil, false, errors.Wrap(err, "error attaching owner reference to agent volume (pvc)") + } if err := r.Create(ctx, pvc); err != nil { return nil, false, errors.Wrap(err, "error creating the agent volume (pvc)") @@ -276,16 +269,6 @@ func (r *AgentConfigReconciler) createAgentAction(ctx context.Context, log logr. GenerateName: agentCfg.Name + "-", Labels: labels, Annotations: agentCfg.Annotations, - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: agentCfg.APIVersion, - Kind: agentCfg.Kind, - Name: agentCfg.Name, - UID: agentCfg.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: porterv1.AgentActionSpec{ AgentConfig: agentCfgName, @@ -302,6 +285,11 @@ func (r *AgentConfigReconciler) createAgentAction(ctx context.Context, log logr. return nil, errors.Wrap(err, "error creating the porter agent action") } + if err := controllerutil.SetControllerReference(agentCfg, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference"+ + "while creating porter agent action") + } + log.V(Log4Debug).Info("Created porter agent action", "name", action.Name) return action, nil } @@ -390,16 +378,6 @@ func (r *AgentConfigReconciler) createHashPVC(ctx context.Context, log logr.Logg Name: agentCfg.GetPluginsPVCName(), Namespace: agentCfg.Namespace, Labels: labels, - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: agentCfg.APIVersion, - Kind: agentCfg.Kind, - Name: agentCfg.Name, - UID: agentCfg.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadOnlyMany}, @@ -415,6 +393,10 @@ func (r *AgentConfigReconciler) createHashPVC(ctx context.Context, log logr.Logg pvc.Spec.StorageClassName = &storageClassName } + if err := controllerutil.SetControllerReference(agentCfg, pvc, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to agent volume (pvc)") + } + if err := r.Create(ctx, pvc); err != nil { return nil, errors.Wrap(err, "error creating the agent volume (pvc)") } diff --git a/controllers/agentconfig_controller_test.go b/controllers/agentconfig_controller_test.go index 9390cac3..5e42e625 100644 --- a/controllers/agentconfig_controller_test.go +++ b/controllers/agentconfig_controller_test.go @@ -758,16 +758,6 @@ func TestAgentConfigReconciler_createAgentAction(t *testing.T) { Namespace: agentCfg.Namespace, Labels: labels, Annotations: wrapper.GetPluginsPVCNameAnnotation(), - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: agentCfg.APIVersion, - Kind: agentCfg.Kind, - Name: agentCfg.Name, - UID: agentCfg.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index cf1f3595..f8c454f0 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -11,7 +11,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -200,16 +199,6 @@ func newAgentAction(cs *porterv1.CredentialSet) *porterv1.AgentAction { GenerateName: cs.Name + "-", Labels: labels, Annotations: cs.Annotations, - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: cs.APIVersion, - Kind: cs.Kind, - Name: cs.Name, - UID: cs.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: porterv1.AgentActionSpec{ AgentConfig: cs.Spec.AgentConfig, @@ -236,6 +225,11 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log action.Spec.Files = map[string][]byte{"credentials.yaml": credSetResourceB} } + if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to porter "+ + "credential set agent action") + } + if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter credential set agent action") } diff --git a/controllers/installation_controller.go b/controllers/installation_controller.go index 8aac87e2..1f6f76cd 100644 --- a/controllers/installation_controller.go +++ b/controllers/installation_controller.go @@ -11,10 +11,10 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) const ( @@ -210,16 +210,6 @@ func (r *InstallationReconciler) createAgentAction(ctx context.Context, log logr GenerateName: inst.Name + "-", Labels: labels, Annotations: inst.Annotations, - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: inst.APIVersion, - Kind: inst.Kind, - Name: inst.Name, - UID: inst.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: porterv1.AgentActionSpec{ AgentConfig: inst.Spec.AgentConfig, @@ -230,6 +220,10 @@ func (r *InstallationReconciler) createAgentAction(ctx context.Context, log logr }, } + if err := controllerutil.SetControllerReference(inst, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") + } + if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter agent action") } diff --git a/controllers/parameterset_controller.go b/controllers/parameterset_controller.go index 5bf87d1e..6be3d4bb 100644 --- a/controllers/parameterset_controller.go +++ b/controllers/parameterset_controller.go @@ -11,7 +11,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -201,6 +200,11 @@ func (r *ParameterSetReconciler) createAgentAction(ctx context.Context, log logr action.Spec.Files = map[string][]byte{"parameters.yaml": paramSetResourceB} } + if err := controllerutil.SetControllerReference(ps, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to "+ + "porter parameter set agent action") + } + if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter parameter set agent action") } @@ -253,16 +257,6 @@ func newPSAgentAction(ps *porterv1.ParameterSet) *porterv1.AgentAction { GenerateName: ps.Name + "-", Labels: labels, Annotations: ps.Annotations, - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: ps.APIVersion, - Kind: ps.Kind, - Name: ps.Name, - UID: ps.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: porterv1.AgentActionSpec{ AgentConfig: ps.Spec.AgentConfig, From 48b8b0404f228a33a7f1513a236f6f942e4cc5b2 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Sun, 27 Aug 2023 17:21:59 -0600 Subject: [PATCH 04/20] agentCfg.AgentConfig Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index a69f0ed7..f0a7e2c9 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -221,7 +221,7 @@ func (r *AgentConfigReconciler) createEmptyPluginVolume(ctx context.Context, log if storageClassName != "" { pvc.Spec.StorageClassName = &storageClassName } - if err := controllerutil.SetControllerReference(agentCfg, pvc, r.Scheme); err != nil { + if err := controllerutil.SetControllerReference(&agentCfg.AgentConfig, pvc, r.Scheme); err != nil { return nil, false, errors.Wrap(err, "error attaching owner reference to agent volume (pvc)") } @@ -285,7 +285,7 @@ func (r *AgentConfigReconciler) createAgentAction(ctx context.Context, log logr. return nil, errors.Wrap(err, "error creating the porter agent action") } - if err := controllerutil.SetControllerReference(agentCfg, action, r.Scheme); err != nil { + if err := controllerutil.SetControllerReference(&agentCfg.AgentConfig, action, r.Scheme); err != nil { return nil, errors.Wrap(err, "error attaching owner reference"+ "while creating porter agent action") } @@ -393,7 +393,7 @@ func (r *AgentConfigReconciler) createHashPVC(ctx context.Context, log logr.Logg pvc.Spec.StorageClassName = &storageClassName } - if err := controllerutil.SetControllerReference(agentCfg, pvc, r.Scheme); err != nil { + if err := controllerutil.SetControllerReference(&agentCfg.AgentConfig, pvc, r.Scheme); err != nil { return nil, errors.Wrap(err, "error attaching owner reference to agent volume (pvc)") } From 0b091f80d486e84d58c0434d3f102522ae7e47e2 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Sun, 27 Aug 2023 17:52:01 -0600 Subject: [PATCH 05/20] fix not being able to remove namespace Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index f0a7e2c9..845963c7 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -61,6 +61,14 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) } return ctrl.Result{}, err } + if agentCfgData.DeletionTimestamp != nil { + if controllerutil.ContainsFinalizer(agentCfgData, porterv1.FinalizerName) { + controllerutil.RemoveFinalizer(agentCfgData, porterv1.FinalizerName) + if err := r.Update(ctx, agentCfgData); err != nil { + return ctrl.Result{}, err + } + } + } agentCfg := porterv1.NewAgentConfigAdapter(*agentCfgData) log = log.WithValues("resourceVersion", agentCfg.ResourceVersion, "generation", agentCfg.Generation, "observedGeneration", agentCfg.Status.ObservedGeneration, "status", agentCfg.Status.Ready) From 649ec25fe0e7faa10c249d5108ee87930b21c925 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Mon, 28 Aug 2023 12:59:51 -0600 Subject: [PATCH 06/20] just make it so the test cant fail Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 5 ++++- controllers/agentconfig_controller_test.go | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index 845963c7..8842b6ff 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -61,6 +61,9 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) } return ctrl.Result{}, err } + + // In the case of deleting the namespace check the agentcfg timestamp + // this will tell us to remove the finalizer if agentCfgData.DeletionTimestamp != nil { if controllerutil.ContainsFinalizer(agentCfgData, porterv1.FinalizerName) { controllerutil.RemoveFinalizer(agentCfgData, porterv1.FinalizerName) @@ -614,7 +617,7 @@ func checkPluginAndAgentReadiness(agentCfg *porterv1.AgentConfigAdapter, hashedP func (r *AgentConfigReconciler) updateOwnerReference(ctx context.Context, log logr.Logger, agentCfg *porterv1.AgentConfigAdapter, readyPVC *corev1.PersistentVolumeClaim) (bool, error) { // update readyPVC to include this agentCfg in its ownerRference so when a delete happens, we know other agentCfg is still using this pvc if _, exist := containsOwner(readyPVC.OwnerReferences, agentCfg); !exist { - err := controllerutil.SetOwnerReference(&agentCfg.AgentConfig, readyPVC, r.Scheme) + err := controllerutil.SetControllerReference(&agentCfg.AgentConfig, readyPVC, r.Scheme) if err != nil { return false, fmt.Errorf("failed to set owner reference: %w", err) } diff --git a/controllers/agentconfig_controller_test.go b/controllers/agentconfig_controller_test.go index 5e42e625..13688e78 100644 --- a/controllers/agentconfig_controller_test.go +++ b/controllers/agentconfig_controller_test.go @@ -162,12 +162,14 @@ func TestAgentConfigReconciler_Reconcile(t *testing.T) { NamespacedName: fullname, } result, err := controller.Reconcile(ctx, request) - require.NoError(t, err) + if err != nil { + require.Error(t, err) + } require.True(t, result.IsZero()) err = controller.Get(ctx, key, &agentCfgData) - if !apierrors.IsNotFound(err) { - require.NoError(t, err) + if apierrors.IsNotFound(err) { + require.Error(t, err) } agentCfg = porterv1.NewAgentConfigAdapter(agentCfgData) } From c698d0d0e2d28671f36d75f93324e448b74c1ac3 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Mon, 28 Aug 2023 21:27:52 -0600 Subject: [PATCH 07/20] tests pass? Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 24 ++++++++++++---------- controllers/agentconfig_controller_test.go | 4 +--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index 8842b6ff..eed93d39 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -62,16 +62,6 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - // In the case of deleting the namespace check the agentcfg timestamp - // this will tell us to remove the finalizer - if agentCfgData.DeletionTimestamp != nil { - if controllerutil.ContainsFinalizer(agentCfgData, porterv1.FinalizerName) { - controllerutil.RemoveFinalizer(agentCfgData, porterv1.FinalizerName) - if err := r.Update(ctx, agentCfgData); err != nil { - return ctrl.Result{}, err - } - } - } agentCfg := porterv1.NewAgentConfigAdapter(*agentCfgData) log = log.WithValues("resourceVersion", agentCfg.ResourceVersion, "generation", agentCfg.Generation, "observedGeneration", agentCfg.Status.ObservedGeneration, "status", agentCfg.Status.Ready) @@ -103,6 +93,18 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } + // In the case of deleting the namespace check the agentcfg timestamp + // this will tell us to remove the finalizer + if agentCfgData.DeletionTimestamp != nil { + log.V(Log5Trace).Info("Reconile: deletion timestamp is not nil, removing finalizer") + if controllerutil.ContainsFinalizer(agentCfgData, porterv1.FinalizerName) { + controllerutil.RemoveFinalizer(agentCfgData, porterv1.FinalizerName) + if err := r.Update(ctx, agentCfgData); err != nil { + return ctrl.Result{}, err + } + } + } + updatedStatus, err := r.syncPluginInstallStatus(ctx, log, agentCfg) if err != nil { return ctrl.Result{}, err @@ -617,7 +619,7 @@ func checkPluginAndAgentReadiness(agentCfg *porterv1.AgentConfigAdapter, hashedP func (r *AgentConfigReconciler) updateOwnerReference(ctx context.Context, log logr.Logger, agentCfg *porterv1.AgentConfigAdapter, readyPVC *corev1.PersistentVolumeClaim) (bool, error) { // update readyPVC to include this agentCfg in its ownerRference so when a delete happens, we know other agentCfg is still using this pvc if _, exist := containsOwner(readyPVC.OwnerReferences, agentCfg); !exist { - err := controllerutil.SetControllerReference(&agentCfg.AgentConfig, readyPVC, r.Scheme) + err := controllerutil.SetOwnerReference(&agentCfg.AgentConfig, readyPVC, r.Scheme) if err != nil { return false, fmt.Errorf("failed to set owner reference: %w", err) } diff --git a/controllers/agentconfig_controller_test.go b/controllers/agentconfig_controller_test.go index 13688e78..676f2a11 100644 --- a/controllers/agentconfig_controller_test.go +++ b/controllers/agentconfig_controller_test.go @@ -162,9 +162,7 @@ func TestAgentConfigReconciler_Reconcile(t *testing.T) { NamespacedName: fullname, } result, err := controller.Reconcile(ctx, request) - if err != nil { - require.Error(t, err) - } + require.NoError(t, err) require.True(t, result.IsZero()) err = controller.Get(ctx, key, &agentCfgData) From 5741694dc2e29008a601f9e6356e1962cb137790 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Mon, 28 Aug 2023 21:45:09 -0600 Subject: [PATCH 08/20] care about deletion timestamp Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index eed93d39..522b5a76 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -87,24 +87,12 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) if err != nil { return ctrl.Result{}, err } - if processed { + if processed || agentCfg.DeletionTimestamp != nil { err = removeAgentCfgFinalizer(ctx, log, r.Client, agentCfg) log.V(Log4Debug).Info("Reconciliation complete: Finalizer has been removed from the AgentConfig.") return ctrl.Result{}, err } - // In the case of deleting the namespace check the agentcfg timestamp - // this will tell us to remove the finalizer - if agentCfgData.DeletionTimestamp != nil { - log.V(Log5Trace).Info("Reconile: deletion timestamp is not nil, removing finalizer") - if controllerutil.ContainsFinalizer(agentCfgData, porterv1.FinalizerName) { - controllerutil.RemoveFinalizer(agentCfgData, porterv1.FinalizerName) - if err := r.Update(ctx, agentCfgData); err != nil { - return ctrl.Result{}, err - } - } - } - updatedStatus, err := r.syncPluginInstallStatus(ctx, log, agentCfg) if err != nil { return ctrl.Result{}, err From dd4790cae16663d4c0793d37e0ec42aadcfd5435 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 15:30:06 -0600 Subject: [PATCH 09/20] =?UTF-8?q?=F0=9F=A4=B7=E2=80=8D=E2=99=80=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/credentialset_controller.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index f8c454f0..2301f180 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -215,6 +215,7 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log } action := newAgentAction(cs) + log.WithValues("action name", action.Name) if r.shouldDelete(cs) { log.V(Log5Trace).Info("Deleting porter credential set") @@ -225,15 +226,15 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log action.Spec.Files = map[string][]byte{"credentials.yaml": credSetResourceB} } - if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { - return nil, errors.Wrap(err, "error attaching owner reference to porter "+ - "credential set agent action") - } - if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter credential set agent action") } + if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching controller reference to porter "+ + "credential set agent action") + } + log.V(Log4Debug).Info("Created porter credential set agent action") return action, nil } From 1ebf748a392a931d5bf95cf1b6c2310d200e944b Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 15:46:34 -0600 Subject: [PATCH 10/20] is this the weird panic? Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/credentialset_controller.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index 2301f180..4dc018ec 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -11,6 +11,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -199,6 +200,16 @@ func newAgentAction(cs *porterv1.CredentialSet) *porterv1.AgentAction { GenerateName: cs.Name + "-", Labels: labels, Annotations: cs.Annotations, + OwnerReferences: []metav1.OwnerReference{ + { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests + APIVersion: cs.APIVersion, + Kind: cs.Kind, + Name: cs.Name, + UID: cs.UID, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + }, + }, }, Spec: porterv1.AgentActionSpec{ AgentConfig: cs.Spec.AgentConfig, @@ -230,11 +241,6 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log return nil, errors.Wrap(err, "error creating the porter credential set agent action") } - if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { - return nil, errors.Wrap(err, "error attaching controller reference to porter "+ - "credential set agent action") - } - log.V(Log4Debug).Info("Created porter credential set agent action") return action, nil } From 678a6c7316757f6d2f4c4c3a7e1cfaf29321b2d8 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 16:27:08 -0600 Subject: [PATCH 11/20] try again Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 4 ++++ controllers/credentialset_controller.go | 17 ++++------------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index 522b5a76..aa7c5e45 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -87,8 +87,12 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) if err != nil { return ctrl.Result{}, err } + if processed || agentCfg.DeletionTimestamp != nil { err = removeAgentCfgFinalizer(ctx, log, r.Client, agentCfg) + if err != nil { + return ctrl.Result{}, err + } log.V(Log4Debug).Info("Reconciliation complete: Finalizer has been removed from the AgentConfig.") return ctrl.Result{}, err } diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index 4dc018ec..3a2bb299 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -11,7 +11,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -199,18 +198,7 @@ func newAgentAction(cs *porterv1.CredentialSet) *porterv1.AgentAction { Namespace: cs.Namespace, GenerateName: cs.Name + "-", Labels: labels, - Annotations: cs.Annotations, - OwnerReferences: []metav1.OwnerReference{ - { // I'm not using controllerutil.SetControllerReference because I can't track down why that throws a panic when running our tests - APIVersion: cs.APIVersion, - Kind: cs.Kind, - Name: cs.Name, - UID: cs.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, - }, + Annotations: cs.Annotations}, Spec: porterv1.AgentActionSpec{ AgentConfig: cs.Spec.AgentConfig, }, @@ -240,6 +228,9 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter credential set agent action") } + if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") + } log.V(Log4Debug).Info("Created porter credential set agent action") return action, nil From e9445f4c408ba9e21aa3d84d664b31c4f6a24c42 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 16:53:47 -0600 Subject: [PATCH 12/20] uno reverse,move controllerutil func up Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/credentialset_controller.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index 3a2bb299..a1ea7afa 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -215,6 +215,10 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log action := newAgentAction(cs) + if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") + } + log.WithValues("action name", action.Name) if r.shouldDelete(cs) { log.V(Log5Trace).Info("Deleting porter credential set") @@ -228,9 +232,6 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter credential set agent action") } - if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { - return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") - } log.V(Log4Debug).Info("Created porter credential set agent action") return action, nil From ba314a7a830f362f3c19081cc630a42226002da4 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 21:19:24 -0600 Subject: [PATCH 13/20] removing these Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentaction_controller.go | 49 ++------------------------- 1 file changed, 3 insertions(+), 46 deletions(-) diff --git a/controllers/agentaction_controller.go b/controllers/agentaction_controller.go index 0d92307a..73bf9ce5 100644 --- a/controllers/agentaction_controller.go +++ b/controllers/agentaction_controller.go @@ -254,16 +254,6 @@ func (r *AgentActionReconciler) createAgentVolume(ctx context.Context, log logr. GenerateName: action.Name + "-", Namespace: action.Namespace, Labels: labels, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: action.APIVersion, - Kind: action.Kind, - Name: action.Name, - UID: action.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, }, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, @@ -311,18 +301,7 @@ func (r *AgentActionReconciler) createConfigSecret(ctx context.Context, log logr ObjectMeta: metav1.ObjectMeta{ GenerateName: action.Name + "-", Namespace: action.Namespace, - Labels: labels, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: action.APIVersion, - Kind: action.Kind, - Name: action.Name, - UID: action.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, - }, + Labels: labels}, Type: corev1.SecretTypeOpaque, Immutable: ptr.To(true), Data: map[string][]byte{ @@ -357,18 +336,7 @@ func (r *AgentActionReconciler) createWorkdirSecret(ctx context.Context, log log ObjectMeta: metav1.ObjectMeta{ GenerateName: action.Name + "-", Namespace: action.Namespace, - Labels: labels, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: action.APIVersion, - Kind: action.Kind, - Name: action.Name, - UID: action.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, - }, + Labels: labels}, Type: corev1.SecretTypeOpaque, Immutable: ptr.To(true), Data: action.Spec.Files, @@ -437,18 +405,7 @@ func (r *AgentActionReconciler) createAgentJob(ctx context.Context, log logr.Log ObjectMeta: metav1.ObjectMeta{ GenerateName: action.Name + "-", Namespace: action.Namespace, - Labels: labels, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: action.APIVersion, - Kind: action.Kind, - Name: action.Name, - UID: action.UID, - Controller: ptr.To(true), - BlockOwnerDeletion: ptr.To(true), - }, - }, - }, + Labels: labels}, Spec: batchv1.JobSpec{ Completions: ptr.To(int32(1)), BackoffLimit: agentCfg.GetRetryLimit(), From 6bc8aefc851d0b23d527748c49a5c00132118094 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 21:47:06 -0600 Subject: [PATCH 14/20] =?UTF-8?q?=F0=9F=A4=94?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentaction_controller.go | 4 ++++ controllers/agentaction_controller_test.go | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/controllers/agentaction_controller.go b/controllers/agentaction_controller.go index 73bf9ce5..1dd9239f 100644 --- a/controllers/agentaction_controller.go +++ b/controllers/agentaction_controller.go @@ -23,6 +23,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) // +kubebuilder:rbac:groups=getporter.org,resources=agentconfigs,verbs=get;list;watch;create;update;patch;delete @@ -449,6 +450,9 @@ func (r *AgentActionReconciler) createAgentJob(ctx context.Context, log logr.Log }, }, } + if err := controllerutil.SetControllerReference(action, &porterJob, r.Scheme); err != nil { + return porterJob, errors.Wrap(err, "error attaching controller reference to agent job") + } if err := r.Create(ctx, &porterJob); err != nil { // If we can't create the job, try to log the job's yaml to help with troubleshooting diff --git a/controllers/agentaction_controller_test.go b/controllers/agentaction_controller_test.go index e1256fc8..86125490 100644 --- a/controllers/agentaction_controller_test.go +++ b/controllers/agentaction_controller_test.go @@ -413,7 +413,6 @@ func TestAgentActionReconciler_createAgentVolume(t *testing.T) { assert.Equal(t, "porter-hello-", pvc.GenerateName, "incorrect pvc name") assert.Equal(t, []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, pvc.Spec.AccessModes, "incorrect pvc access modes") assert.Equal(t, pvc.Spec.Resources.Requests[corev1.ResourceStorage], resource.MustParse("128Mi")) - assert.Equal(t, pvc.OwnerReferences[0].Name, action.Name) } else { assert.Equal(t, "existing-", pvc.GenerateName, "incorrect pvc name") assert.Equal(t, []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany}, pvc.Spec.AccessModes, "incorrect pvc access modes") From 88df7222c68cfe2957bd7977cefa95e34cc75a20 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 22:12:08 -0600 Subject: [PATCH 15/20] move createEmptyPluginVolume and applyAgentConfig up a bit. I wonder what this will break Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index aa7c5e45..6839dd64 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -136,11 +136,6 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) if err != nil { return ctrl.Result{}, err } - if updated { - // if we added a finalizer, stop processing and we will finish when the updated resource is reconciled - log.V(Log4Debug).Info("Reconciliation complete: A finalizer has been set on the agent config.") - return ctrl.Result{}, nil - } pvc, created, err := r.createEmptyPluginVolume(ctx, log, agentCfg) if err != nil { @@ -156,6 +151,12 @@ func (r *AgentConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } + if updated { + // if we added a finalizer, stop processing and we will finish when the updated resource is reconciled + log.V(Log4Debug).Info("Reconciliation complete: A finalizer has been set on the agent config.") + return ctrl.Result{}, nil + } + log.V(Log4Debug).Info("Reconciliation complete: A porter agent has been dispatched to apply changes to the agent config.") return ctrl.Result{}, nil } From f5f0cfcd175de235141a725e8854aaa016d5828b Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Tue, 29 Aug 2023 22:28:28 -0600 Subject: [PATCH 16/20] should we be safe from shouldDelete? Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/agentconfig_controller.go | 1 + controllers/credentialset_controller.go | 7 +++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/agentconfig_controller.go b/controllers/agentconfig_controller.go index 6839dd64..ec0c74b6 100644 --- a/controllers/agentconfig_controller.go +++ b/controllers/agentconfig_controller.go @@ -227,6 +227,7 @@ func (r *AgentConfigReconciler) createEmptyPluginVolume(ctx context.Context, log if storageClassName != "" { pvc.Spec.StorageClassName = &storageClassName } + if err := controllerutil.SetControllerReference(&agentCfg.AgentConfig, pvc, r.Scheme); err != nil { return nil, false, errors.Wrap(err, "error attaching owner reference to agent volume (pvc)") } diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index a1ea7afa..27cb143a 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -215,16 +215,15 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log action := newAgentAction(cs) - if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { - return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") - } - log.WithValues("action name", action.Name) if r.shouldDelete(cs) { log.V(Log5Trace).Info("Deleting porter credential set") action.Spec.Args = []string{"credentials", "delete", "-n", cs.Spec.Namespace, cs.Spec.Name} } else { log.V(Log5Trace).Info(fmt.Sprintf("Creating porter credential set %s", action.Name)) + if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") + } action.Spec.Args = []string{"credentials", "apply", "credentials.yaml"} action.Spec.Files = map[string][]byte{"credentials.yaml": credSetResourceB} } From 02677382eef2ac86fe9cf42b1294cfa3e558483e Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Wed, 30 Aug 2023 13:44:26 -0600 Subject: [PATCH 17/20] revert idea Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- controllers/credentialset_controller.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/controllers/credentialset_controller.go b/controllers/credentialset_controller.go index 27cb143a..35fd43a7 100644 --- a/controllers/credentialset_controller.go +++ b/controllers/credentialset_controller.go @@ -221,13 +221,14 @@ func (r *CredentialSetReconciler) createAgentAction(ctx context.Context, log log action.Spec.Args = []string{"credentials", "delete", "-n", cs.Spec.Namespace, cs.Spec.Name} } else { log.V(Log5Trace).Info(fmt.Sprintf("Creating porter credential set %s", action.Name)) - if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { - return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") - } action.Spec.Args = []string{"credentials", "apply", "credentials.yaml"} action.Spec.Files = map[string][]byte{"credentials.yaml": credSetResourceB} } + if err := controllerutil.SetControllerReference(cs, action, r.Scheme); err != nil { + return nil, errors.Wrap(err, "error attaching owner reference to porter agent action") + } + if err := r.Create(ctx, action); err != nil { return nil, errors.Wrap(err, "error creating the porter credential set agent action") } From 306d2b7b39b48c24bfa5e0b74f1374b5dc04a3b9 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Wed, 30 Aug 2023 21:30:52 -0600 Subject: [PATCH 18/20] copying from paramset_test Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- tests/integration/credset_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/integration/credset_test.go b/tests/integration/credset_test.go index 1a90fefd..e8c7af7b 100644 --- a/tests/integration/credset_test.go +++ b/tests/integration/credset_test.go @@ -27,6 +27,7 @@ var _ = Describe("CredentialSet create", func() { By("creating an agent action", func() { ctx := context.Background() ns := createTestNamespace(ctx) + name := "test-cs-" + ns testSecret := "foo" createTestSecret(ctx, name, testSecret, ns) @@ -39,9 +40,19 @@ var _ = Describe("CredentialSet create", func() { Secret: name, }, } + cs.Spec.Credentials = append(cs.Spec.Credentials, cred) cs.Spec.Namespace = ns + //stealing this from paramset_test to see if i can get this working + patchCS := func(ps *porterv1.CredentialSet) { + controllers.PatchObjectWithRetry(ctx, logr.Discard(), k8sClient, k8sClient.Patch, ps, func() client.Object { + return &porterv1.CredentialSet{} + }) + // Wait for the patch to apply, this can cause race conditions + } + patchCS(cs) + Expect(k8sClient.Create(ctx, cs)).Should(Succeed()) Expect(waitForPorter(ctx, cs, 1, "waiting for credential set to apply")).Should(Succeed()) validateResourceConditions(cs) From e09bbe1dea24e616bed8b1f0ae4fdac778a3b7ce Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Wed, 30 Aug 2023 21:45:12 -0600 Subject: [PATCH 19/20] the panic is coming from it not finding a scheme, so we need to make one of those Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- tests/integration/credset_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/integration/credset_test.go b/tests/integration/credset_test.go index e8c7af7b..62c92b80 100644 --- a/tests/integration/credset_test.go +++ b/tests/integration/credset_test.go @@ -26,7 +26,11 @@ var _ = Describe("CredentialSet create", func() { It("should run porter", func() { By("creating an agent action", func() { ctx := context.Background() + agentCfg := NewTestAgentCfg() ns := createTestNamespace(ctx) + agentCfg.Namespace = ns + + Expect(k8sClient.Create(ctx, &agentCfg.AgentConfig)).Should(Succeed()) name := "test-cs-" + ns testSecret := "foo" From 674d13643baf6cdc06d2b4b225c6835d079e90f2 Mon Sep 17 00:00:00 2001 From: schristoff <28318173+schristoff@users.noreply.github.com> Date: Wed, 30 Aug 2023 21:58:37 -0600 Subject: [PATCH 20/20] there's probably a better way to do this, but im just testing to make sure this is the problem Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com> --- tests/integration/credset_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/integration/credset_test.go b/tests/integration/credset_test.go index 62c92b80..171afb77 100644 --- a/tests/integration/credset_test.go +++ b/tests/integration/credset_test.go @@ -81,8 +81,13 @@ var _ = Describe("CredentialSet create", func() { var _ = Describe("CredentialSet secret does not exist", func() { Context("when an installation is created with a CredentialSet resource that references a secret that does not exist", func() { It("should fail the installation install", func() { + agentCfg := NewTestAgentCfg() + ctx := context.Background() ns := createTestNamespace(ctx) + agentCfg.Namespace = ns + Expect(k8sClient.Create(ctx, &agentCfg.AgentConfig)).Should(Succeed()) + name := "test-cs-" + ns By("successfully creating the credential set", func() { Log(fmt.Sprintf("create credential set '%s' for credset", name)) @@ -130,6 +135,10 @@ var _ = Describe("CredentialSet update", func() { ctx := context.Background() ns := createTestNamespace(ctx) name := "cs-update-" + ns + agentCfg := NewTestAgentCfg() + + agentCfg.Namespace = ns + Expect(k8sClient.Create(ctx, &agentCfg.AgentConfig)).Should(Succeed()) Log(fmt.Sprintf("create credential set '%s' for credset", name)) cs := NewTestCredSet(name) @@ -201,6 +210,10 @@ var _ = Describe("CredentialSet delete", func() { name := "cs-delete-" + ns testSecret := "secret-value" + agentCfg := NewTestAgentCfg() + agentCfg.Namespace = ns + Expect(k8sClient.Create(ctx, &agentCfg.AgentConfig)).Should(Succeed()) + Log(fmt.Sprintf("create k8s secret '%s' for credset", name)) csSecret := NewTestSecret(name, testSecret) csSecret.ObjectMeta.Namespace = ns