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.

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

Update Emergency Tier's Priority to 50 #1665

Merged
merged 8 commits into from
Dec 22, 2020
Merged

Conversation

abhiraut
Copy link
Contributor

@abhiraut abhiraut commented Dec 16, 2020

This PR updates the priority of Tiers created by Antrea, and space them out evenly. The current Antrea generated Tier priorities do not allow enough room for user defined Tiers to be created, for example Emergency Tier allows only 4 user created Tiers with higher priority. Since it ultimately is up to the users as to which Tiers go atop and the purpose of Antrea created Tiers is mainly that of convenience, we decided to distribute the priorities more evenly across available priority space. For example, setting the priority of the top most Tier of Emergency to a value of 50 gives enough room for admins to create their own Tier hierarchy without having to rely on Antrea created Tiers.

Summary of change:

Tier Name        Old Priority        Updated Priority
emergency         5                         50
securityops       50                        100
networkops        100                       150
platform          150                       200

NOTE: Existing users of Antrea created Tiers are advised to re-evaluate their Tier hierarchy and take action to recreate any user created Tier resource, such that they continue to be enforced as per user's intention.

@codecov-io
Copy link

codecov-io commented Dec 16, 2020

Codecov Report

Merging #1665 (470dc8e) into master (9d3d10b) will increase coverage by 0.46%.
The diff coverage is 56.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1665      +/-   ##
==========================================
+ Coverage   63.31%   63.78%   +0.46%     
==========================================
  Files         170      181      +11     
  Lines       14250    16072    +1822     
==========================================
+ Hits         9023    10252    +1229     
- Misses       4292     4772     +480     
- Partials      935     1048     +113     
Flag Coverage Δ
e2e-tests 45.77% <31.70%> (?)
kind-e2e-tests 53.64% <53.38%> (-1.75%) ⬇️
unit-tests 40.60% <28.68%> (-0.68%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/antrea-agent/agent.go 0.00% <0.00%> (ø)
.../agent/apiserver/handlers/networkpolicy/handler.go 58.33% <ø> (ø)
...gent/controller/noderoute/node_route_controller.go 60.98% <0.00%> (+14.51%) ⬆️
pkg/agent/stats/collector.go 97.72% <ø> (ø)
pkg/antctl/antctl.go 100.00% <ø> (ø)
pkg/antctl/transform/controllerinfo/transform.go 0.00% <ø> (ø)
pkg/antctl/transform/version/transform.go 0.00% <ø> (ø)
pkg/controller/types/networkpolicy.go 100.00% <ø> (ø)
pkg/features/antrea_features.go 16.66% <ø> (ø)
pkg/ovs/openflow/ofctrl_builder.go 60.94% <0.00%> (-1.23%) ⬇️
... and 111 more

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

I think it would be good to add the rationale for this change in the commit message / PR description.

Also it seems that in case of an Antrea update, the priority for the emergency tier will stay the same (at 5) and not be updated (to 20). Is that acceptable?

@abhiraut
Copy link
Contributor Author

abhiraut commented Dec 16, 2020

I think it would be good to add the rationale for this change in the commit message / PR description.

Also it seems that in case of an Antrea update, the priority for the emergency tier will stay the same (at 5) and not be updated (to 20). Is that acceptable?

apologies i wanted to keep it WIP. im thinking of force update priority to 20 for existing Emergency Tiers. However, that has the inherent issue of it being demoted below other user created CRDs whose priority lies between 6-19. I am wondering if that is acceptable. May need a release note.

@abhiraut abhiraut changed the title Update Emergency Tier's Priority to 20 WIP: Update Emergency Tier's Priority to 20 Dec 16, 2020
@abhiraut abhiraut changed the title WIP: Update Emergency Tier's Priority to 20 Update Emergency Tier's Priority to 20 Dec 16, 2020
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.

How many tiers in total we support?
Feel something like {50, 100, 150, 200} sounds better, but maybe you want to change all tiers.

@abhiraut
Copy link
Contributor Author

This PR updates the priority of Emergency Tier created by Antrea, and set it to 20 as opposed to the intially assigned value of
5. The current Antrea generated Emergency Tier priority of 5 does not allow enough room for user defined Tiers to be created
with higher priority, as ultimately it is up to the users as to which Tiers go atop and the purpose of Antrea created Tiers is
mainly that of convenience. Setting the priority of the top most Tier of Emergency to a value of 20 gives enough room for
admins to create their own Tier hierarchy without having to rely on Antrea created Tiers.

How many tiers in total we support?
Feel something like {50, 100, 150, 200} sounds better, but maybe you want to change all tiers.

Theoretically we can support up to 255. Practically we put a soft limit of 20 Tiers, but this limit of 20 can be relaxed with a single LOC change in validation.

Current Tier priorities after this change is as follows:

emergency     20         
securityops   50         
networkops    100        
platform      150        
application   250        

we could do:

emergency     50         
securityops   100         
networkops    150        
platform      200        
application   250        

Let me know if you prefer to keep proposal 1 or change to proposal 2.

@jianjuns
Copy link
Contributor

Ok. Seems I missed application tier. Any reason we go from 150 to 200 for application tier today?

This combination looks easier to understand for me:
emergency 50
securityops 100
networkops 150
platform 200
application 250

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Not a big fan of a change like this, but I guess it can be good to have more evenly-spaced out system tier priorities, and the API is still in alpha.

pkg/controller/networkpolicy/tier.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/tier.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
@Dyanngg
Copy link
Contributor

Dyanngg commented Dec 17, 2020

The PR description also needs update

pkg/util/env/env.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
@abhiraut
Copy link
Contributor Author

/test-all

antoninbas
antoninbas previously approved these changes Dec 18, 2020
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Some nits, otherwise LGTM

Maybe @tnqn would like to take a quick look in case he has comments on the userInfo approach I suggested?

pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
pkg/util/env/env.go Outdated Show resolved Hide resolved
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.

LGTM

antoninbas
antoninbas previously approved these changes Dec 18, 2020
@abhiraut
Copy link
Contributor Author

/test-all

pkg/controller/networkpolicy/tier.go Outdated Show resolved Hide resolved
pkg/controller/networkpolicy/tier.go Outdated Show resolved Hide resolved
@Dyanngg Dyanngg changed the title Update Emergency Tier's Priority to 20 Update Emergency Tier's Priority to 50 Dec 18, 2020
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@abhiraut
Copy link
Contributor Author

/test-all

@abhiraut abhiraut requested a review from tnqn December 22, 2020 00:15
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

@abhiraut abhiraut merged commit 7ce1c75 into antrea-io:master Dec 22, 2020
@abhiraut abhiraut deleted the emer-prio branch December 22, 2020 05:08
antoninbas pushed a commit that referenced this pull request Dec 23, 2020
This PR updates the priority of Tiers created by Antrea, and space them out evenly. The current Antrea generated Tier
priorities do not allow enough room for user defined Tiers to be created, for example Emergency Tier allows only 4 user
created Tiers with higher priority. Since it ultimately is up to the users as to which Tiers go atop and the purpose of
Antrea created Tiers is mainly that of convenience, we decided to distribute the priorities more evenly across available
priority space. For example, setting the priority of the top most Tier of Emergency to a value of 50 gives enough room for
admins to create their own Tier hierarchy without having to rely on Antrea created Tiers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants