Remove the pod reference in v1beta1 AddressGroup #1586
Conversation
|
Thanks for your PR. The following commands are available:
|
Codecov Report
@@ Coverage Diff @@
## master #1586 +/- ##
==========================================
+ Coverage 63.31% 64.01% +0.69%
==========================================
Files 170 181 +11
Lines 14250 15556 +1306
==========================================
+ Hits 9023 9958 +935
- Misses 4292 4547 +255
- Partials 935 1051 +116
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/test-all |
|
/test-all |
|
/test-windows-conformance |
tnqn
force-pushed
the
v1beta1-revert
branch
2 times, most recently
from
39fd4b8
to
83891c4
Compare
|
/test-all |
To be backwards compatible, AddressGroup v1beta1 must not include PodReferences in its members, otherwise the agents that use v1beta1 will think the members have changed and will add new IPs to the openflow entries of relevant NetworkPolicies and delete old IPs from them, while the old IPs and new IPs are actually the same. In current implementation, deleting old IPs is executed after adding new IPs, so it leads to some IPs missing in the openflow entries.
| if err = data.deleteNetworkpolicy(np); err != nil { | ||
| t.Fatalf("Error when deleting network policy: %v", err) | ||
| } | ||
| }() | ||
| }) | ||
| time.Sleep(networkPolicyDelay) |
There was a problem hiding this comment.
Not sure if it's changing the podSelector to "MatchExpressions" caused the networkpolicy takes a little more time to realize. The test now has a chance to fail without the sleep, I can confirm it's not because of the conversion function change as it also failed when I just changed the test code in this PR: https://github.com/vmware-tanzu/antrea/runs/1431829031?check_suite_focus=true
|
/test-all |
|
/test-e2e |
| // conversion is called for an AddressGroup as agents don't expect it in v1beta1 version. | ||
| // This function doesn't match the pattern of conversion function which requires the last parameter to be | ||
| // conversion.Scope so won't be registered to schema. | ||
| func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, includePodRef bool) error { |
There was a problem hiding this comment.
could have been a private function?
There was a problem hiding this comment.
Definitely can be private, I kept its name as it was as code in this file doesn't follow normal go style anyway.
Do you prefer it to be private?
| // This function doesn't match the pattern of conversion function which requires the last parameter to be | ||
| // conversion.Scope so won't be registered to schema. |
There was a problem hiding this comment.
BTW, is there any advantage to having conversion functions for non resources registered to the schema? I understand why Convert_controlplane_AddressGroup_To_v1beta1_AddressGroup needs to be registered, but it seems conversion functions for non resources will only be called indirectly by other conversion functions. Is it for auto-generated conversion functions?
There was a problem hiding this comment.
Good question, I wondered this as well, haven't found answer so blindly followed what K8s code does.
I can take a look what it would affect later and see if we could remove them. Created #1616 to track it.
To be backwards compatible, AddressGroup v1beta1 must not include PodReferences in its members, otherwise the agents that use v1beta1 will think the members have changed and will add new IPs to the openflow entries of relevant NetworkPolicies and delete old IPs from them, while the old IPs and new IPs are actually the same. In current implementation, deleting old IPs is executed after adding new IPs, so it leads to some IPs missing in the openflow entries. Fixes antrea-io#1587
To be backwards compatible, AddressGroup v1beta1 must not include PodReferences in its members, otherwise the agents that use v1beta1 will think the members have changed and will add new IPs to the openflow entries of relevant NetworkPolicies and delete old IPs from them, while the old IPs and new IPs are actually the same. In current implementation, deleting old IPs is executed after adding new IPs, so it leads to some IPs missing in the openflow entries. Fixes #1587
To be backwards compatible, AddressGroup v1beta1 must not include PodReferences in its members, otherwise the agents that use v1beta1 will think the members have changed and will add new IPs to the openflow entries of relevant NetworkPolicies and delete old IPs from them, while the old IPs and new IPs are actually the same. In current implementation, deleting old IPs is executed after adding new IPs, so it leads to some IPs missing in the openflow entries. Fixes #1587
To be backwards compatible, AddressGroup v1beta1 must not include
PodReferences in its members, otherwise the agents that use v1beta1 will
think the members have changed and will add new IPs to the openflow
entries of relevant NetworkPolicies and delete old IPs from them, while
the old IPs and new IPs are actually the same. In current
implementation, deleting old IPs is executed after adding new IPs, so it
leads to some IPs missing in the openflow entries.
Fixes #1587