Update ResourceExport's json tag as lowerCamelCase #4211
Conversation
|
/test-multicluster-e2e |
Codecov Report
@@ Coverage Diff @@
## main #4211 +/- ##
==========================================
+ Coverage 58.30% 61.47% +3.16%
==========================================
Files 384 395 +11
Lines 54376 54447 +71
==========================================
+ Hits 31702 33469 +1767
+ Misses 20257 18511 -1746
- Partials 2417 2467 +50
*This pull request uses carry forward flags. Click here to find out more.
|
Change ResourceExport's json tag to lowerCamelCase. Since any existing ResourceExport/ResourceImport's spec will be empty if it's ClusterInfo or ClusterNetworkPolicy type after this change, add a few steps to skip importing any ResoureImport if it doesn't have valid spec. Signed-off-by: Lan Luo <[email protected]>
luolanzone
force-pushed
the
update-resourceexport-tags
branch
from
1032511
to
d62a4ca
Compare
| @@ -43,6 +43,10 @@ var ( | |||
| ) | |||
|
|
|||
| func (r *ResourceImportReconciler) handleResImpUpdateForClusterNetworkPolicy(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) { | |||
| if resImp.Spec.ClusterNetworkPolicy == nil { | |||
There was a problem hiding this comment.
Questions:
- Do we need this check for ResourceExport reconcilers too?
- But what will happen if this ResourceImport was not processed before mc-controller upgrade? After such change, we should either recreate all impacted resources or support old version?
There was a problem hiding this comment.
We can skip reconciling ResourceExport if spec is empty, but the main purpose of this change is to avoid controller crash for any existing ResourceImport with empty spec. When users deploy latest manifest with this change, the existing ResourceImport spec will become empty due to mismatch json tag.
When users upgrade all member and leader clusters, they need to restart controller manually, then all impacted resources will be updated automatically because ResourceExports will be updated by member controllers.
There was a problem hiding this comment.
I am thinking maybe we can add an empty line in the config file so that the ConfigMap checksum annotation can be updated. In this case, user don't have to restart controller manually. Deployment will be updated and restarted automatically due to annotation change. What's your thought?
There was a problem hiding this comment.
After upgrading from 1.8.0 to 1.9.0 which includes this change, mc-controller will be restarted. Why manual restart is required? But could you confirm in the current implementation, mc-controller will update all existing ResourceExports after restart? Could you verify that?
There was a problem hiding this comment.
Oh, yes, you are right. For this standalone PR, the manifest change is for CRD part only, so controller won't be restarted. But for a new release 1.9, mc-controller will be restarted.
I verified this locally, when the leader CRD is updated, the spec of existing ResourceExport will be empty, but it will be updated soon after member controller restart since there are difference between new ResourceExport and existing one.
There was a problem hiding this comment.
So, will ResourceImport be recreated too? How about imported resources in the member cluster?
There was a problem hiding this comment.
The corresponding ResourceImport will be updated, then the member controller will be notified and handles the update event properly to refresh imported resources.
|
/test-multicluster-e2e |
Change ResourceExport's json tag to lowerCamelCase. After antrea-mc-controller is upgraded from an older version to a version with this change, it will get an empty spec for an existing existing ResourceExport/ResourceImport of type ClusterInfo or ClusterNetworkPolicy type, so we add code to skip importing any ResoureImport if it doesn't have a valid spec. Signed-off-by: Lan Luo <[email protected]>
Change ResourceExport's json tag to lowerCamelCase. After
antrea-mc-controller is upgraded from an older version to a version
with this change, it will get an empty spec for an existing existing
ResourceExport/ResourceImport of type ClusterInfo or
ClusterNetworkPolicy type, so we add code to skip importing any
ResoureImport if it doesn't have a valid spec.
Signed-off-by: Lan Luo [email protected]