Skip to content
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

Jump to bottom

Network Policy Controller sync #82

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Network Policy Controller sync #82

merged 1 commit into from
Nov 18, 2019

Conversation

abhiraut
Copy link
Contributor

Along with namespace event handlers, add
logic to sync Groups and internal NP
objects.

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /recheck: to re-trigger unit tests and code linters. This is done automatically when the PR is updated (e.g. a new commit is added) but this command is useful in case of transient errors with the CI system.
  • /test-e2e: to trigger e2e tests. This command can only be run by members of the vmware-tanzu organization

@tnqn tnqn mentioned this pull request Nov 15, 2019
Closed
8 tasks
@abhiraut
Copy link
Contributor Author

/test-e2e

@abhiraut abhiraut mentioned this pull request Nov 16, 2019
Merged
@abhiraut abhiraut changed the base branch from abhiraut/np-handlers to master November 17, 2019 06:34
tnqn
tnqn reviewed Nov 17, 2019
View reviewed changes
pkg/controller/networkpolicy/networkpolicy_controller.go
// Retrieve all Pods matching the podSelector.
pods, err = n.podLister.Pods(appliedToGroup.Selector.Namespace).List(selector)
for _, pod := range pods {
if pod.Status.PodIP == "" {
Copy link
Member

Choose a reason for hiding this comment

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

L672 needs to check whether IP changes too.

Copy link
Contributor

Choose a reason for hiding this comment

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

If Pod without IP typically happens maybe check and ignore Pods with no IP in addPod()?

Around line 662, we could also optimize not to get both old and new groups if Pod labels do not change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are checking for PodIP in add/updatePod.. though this check is also needed because we are retrieving a list of pods from lister which may return pods with no IP.

labels matching is already been done around L662 .. we return if no label, nodename or pod ip change

jianjuns
jianjuns reviewed Nov 17, 2019
View reviewed changes
pkg/controller/networkpolicy/networkpolicy_controller.go
if groupSelector.Namespace != "" {
// Namespace presence indicates Pods must be selected from the same Namespace.
pods, _ = n.podLister.Pods(groupSelector.Namespace).List(pSelector)
} else if groupSelector.NamespaceSelector != nil && groupSelector.PodSelector != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe check groupSelector.PodSelector first.

pkg/controller/networkpolicy/networkpolicy_controller.go Show resolved Hide resolved
pkg/controller/networkpolicy/networkpolicy_controller.go
// Retrieve all Pods matching the podSelector.
pods, err = n.podLister.Pods(appliedToGroup.Selector.Namespace).List(selector)
for _, pod := range pods {
if pod.Status.PodIP == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

If Pod without IP typically happens maybe check and ignore Pods with no IP in addPod()?

Around line 662, we could also optimize not to get both old and new groups if Pod labels do not change?

jianjuns
jianjuns reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

In addPod() could we ignore Pods if no IP set? Assuming a Pod is always first seen without IP, it could be an optimization.

I also add several other comments, but all these are optimization, and I am fine to address them later.

pkg/controller/networkpolicy/networkpolicy_controller.go
@@ -627,9 +661,9 @@ func (n *NetworkPolicyController) updatePod(oldObj, curObj interface{}) {
// Create set to hold the group keys to enqueue.
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant if labelsEqual is true, then line 659 and 660 could be skipped.

@abhiraut
Copy link
Contributor Author

/test-e2e

@abhiraut
Add Group and internal NP workers
58c47ea 
Along with namespace event handlers, add logic to sync Groups and
internal NP objects.
tnqn
tnqn approved these changes Nov 18, 2019
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM now

@abhiraut abhiraut merged commit 6e33a61 into master Nov 18, 2019
@abhiraut abhiraut deleted the abhiraut/np-integration branch November 18, 2019 08:57
zyiou added a commit to zyiou/antrea that referenced this pull request Jul 2, 2021
@zyiou
modify go.mod (antrea-io#82)
45b6c18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianjuns jianjuns jianjuns left review comments

@tnqn tnqn tnqn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abhiraut @antrea-bot @tnqn @jianjuns