openshift / aws-efs-operator

Operator to manage AWS EFS on OpenShift
Apache License 2.0
3 stars 23 forks source link

Update go dependencies #43

Closed cblecker closed 2 years ago

cblecker commented 2 years ago
jharrington22 commented 2 years ago

@cblecker Nice! :bow:

I'll take a look at the test tomorrow.

jharrington22 commented 2 years ago

@cblecker figured out the test issues.

The sharedvolume test delete's the PersistentVolume to test the uneditSharedVolume corner cases (immutable PVs since Dedicated Admin's shouldn't be able to delete them). When it does this the sharedvolume controller see's the PV delete event, queues a sharedvolume reconcile which validates there is no PV, requeues another sharedvolume reconcile with no error which then Ensures the PV is replaced.

This causes the cached version of the PV and the server version of the PV in the test to become out of sync causing the Operation cannot be fulfilled on persistentvolumes "pv-namespace-y-sv-b": object was modified error.

We need to add the following to fix the test:

diff --git a/pkg/controller/sharedvolume/sharedvolume_controller_test.go b/pkg/controller/sharedvolume/sharedvolume_controller_test.go
index fcaf63f..137271e 100644
--- a/pkg/controller/sharedvolume/sharedvolume_controller_test.go
+++ b/pkg/controller/sharedvolume/sharedvolume_controller_test.go
@@ -347,9 +347,14 @@ func TestReconcile(t *testing.T) {
        if res, err = r.Reconcile(req); res != test.NullResult || err != nil {
                t.Fatalf("Expected no requeue, no error; got\nresult: %v\nerr: %v", res, err)
        }
+
        // validateResources proves the PV and PVC came back.
        svMap, pvMap, _ = validateResources(t, r.client, 2)

+       // The PV came back but we need to update the local cached version of the PV since its at
+       // revision 2 and the server version after the delete and Ensure is now at revision 1
+       pv = pvMap[pvname]
+
        // Hit some uneditSharedVolume corner cases. These will panic in uneditSharedVolume, which is
        // recovered and spoofed as a non-error on the theory that the main Reconcile should overwrite
        // the PV. But it won't do that, because PVs can't be edited (which means we shouldn't get here
jharrington22 commented 2 years ago

Just noticed the statics test is also failing.

jharrington22 commented 2 years ago

@cblecker because of the logger changes we need to remove the ZapLogger refs.

diff --git a/pkg/controller/statics/statics_controller_test.go b/pkg/controller/statics/statics_controller_test.go
index 023d91c..d5036d1 100644
--- a/pkg/controller/statics/statics_controller_test.go
+++ b/pkg/controller/statics/statics_controller_test.go
@@ -27,7 +27,6 @@ import (
 // TODO: Test add()/watches somehow?

 func setup() (logr.Logger, *ReconcileStatics) {
-       logf.SetLogger(logf.ZapLogger(true))

        // OpenShift types need to be registered explicitly
        scheme.Scheme.AddKnownTypes(securityv1.SchemeGroupVersion, &securityv1.SecurityContextConstraints{})
diff --git a/pkg/controller/statics/statics_test.go b/pkg/controller/statics/statics_test.go
index 83dcf1d..465329a 100644
--- a/pkg/controller/statics/statics_test.go
+++ b/pkg/controller/statics/statics_test.go
@@ -24,7 +24,6 @@ func TestEnsureStatics(t *testing.T) {
        // Future-proof this test against new statics being added.
        checkNumStatics(t)

-       logf.SetLogger(logf.ZapLogger(true))
        logger := logf.Log.Logger
        ctx := context.TODO()
jharrington22 commented 2 years ago

:ok_hand:

GOOS=linux GOARCH=amd64 CGO_ENABLED=0 GOFLAGS= go test  openshift/aws-efs-operator/cmd/manager openshift/aws-efs-operator/config openshift/aws-efs-operator/pkg/apis openshift/aws-efs-operator/pkg/apis/awsefs openshift/aws-efs-operator/pkg/apis/awsefs/v1alpha1 openshift/aws-efs-operator/pkg/controller openshift/aws-efs-operator/pkg/controller/sharedvolume openshift/aws-efs-operator/pkg/controller/statics openshift/aws-efs-operator/pkg/fixtures openshift/aws-efs-operator/pkg/test openshift/aws-efs-operator/pkg/util openshift/aws-efs-operator/version

?       openshift/aws-efs-operator/cmd/manager  [no test files]
?       openshift/aws-efs-operator/config       [no test files]
?       openshift/aws-efs-operator/pkg/apis     [no test files]
?       openshift/aws-efs-operator/pkg/apis/awsefs      [no test files]
?       openshift/aws-efs-operator/pkg/apis/awsefs/v1alpha1     [no test files]
?       openshift/aws-efs-operator/pkg/controller       [no test files]
ok      openshift/aws-efs-operator/pkg/controller/sharedvolume  0.040s
ok      openshift/aws-efs-operator/pkg/controller/statics       0.054s
?       openshift/aws-efs-operator/pkg/fixtures [no test files]
?       openshift/aws-efs-operator/pkg/test     [no test files]
ok      openshift/aws-efs-operator/pkg/util     0.011s
?       openshift/aws-efs-operator/version      [no test files]
codecov-commenter commented 2 years ago

Codecov Report

Merging #43 (74453d2) into master (f4bff3f) will increase coverage by 0.17%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   75.98%   76.16%   +0.17%     
==========================================
  Files          20       21       +1     
  Lines         683      688       +5     
==========================================
+ Hits          519      524       +5     
  Misses        157      157              
  Partials        7        7              
Impacted Files Coverage Δ
...controller/sharedvolume/sharedvolume_controller.go 86.28% <100.00%> (ø)
pkg/util/slice.go 100.00% <100.00%> (ø)
jharrington22 commented 2 years ago

Nice, I'll now test on cluster.

openshift-ci[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, jharrington22

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/aws-efs-operator/blob/master/OWNERS)~~ [jharrington22] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment