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

Support for GlobalTrafficPolicy priority processing #249

Merged
merged 3 commits into from
Jul 27, 2022

Conversation

vrushalijoshi
Copy link
Collaborator

Adding support to handle GlobalTrafficPolicy with priority label

  1. Added label for resource priority processing (currently used for only processing GlobalTrafficPolicies)
  2. Code updates to handle priority labeled GlobalTrafficPolicy resource, and pick the most recently created one if there are multiple priority labeled GTPs.

Copy link
Contributor

@aattuluri aattuluri left a comment

Choose a reason for hiding this comment

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

Please take a look at the comments.

@@ -115,6 +115,8 @@ func GetRootCmd(args []string) *cobra.Command {
"The label value, on a namespace or service, which tells Istio to perform sidecar injection")
rootCmd.PersistentFlags().StringVar(&params.LabelSet.AdmiralIgnoreLabel, "admiral_ignore_label", "admiral-ignore",
"The label value, on a namespace, which tells Istio to perform sidecar injection")
rootCmd.PersistentFlags().StringVar(&params.LabelSet.ResourcePriorityKey, "resource_priority_key", "admiral.io/resource-priority",
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply call it priority instead of resource priority.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated to priority label

@@ -40,6 +40,7 @@ func init() {

p.LabelSet.WorkloadIdentityKey = "identity"
p.LabelSet.GlobalTrafficDeploymentLabel = "identity"
p.LabelSet.ResourcePriorityKey = "admiral.io/resource-priority"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated to priority label

@@ -276,6 +290,15 @@ func updateGlobalGtpCache(cache *AdmiralCache, identity, env string, gtps map[st
}
}

func sortFetchedGtps(gtpsToOrder []*v1.GlobalTrafficPolicy, identity string, env string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func sortFetchedGtps(gtpsToOrder []*v1.GlobalTrafficPolicy, identity string, env string) {
func sortGtpsByCreationTime(gtpsToOrder []*v1.GlobalTrafficPolicy, identity string, env string) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the method name

}
mostRecentGtp = priorityGtpsOrdered[0]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we build this into sorting function itself? Sort by priority label present followed by date created?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the sorting function

}
} else {
for _, matchedGtp := range gtpsOrdered {
if matchedGtp.ObjectMeta.Labels[common.GetAdmiralParams().LabelSet.ResourcePriorityKey] == "true" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vrushalijoshi , just a thought. This would mean that any GTP created by "anyone" with admiral.io/resource-priority: true would be treated as a higher priority.
Do you think it would make sense to have a value that could be passed as a param when the admiral starts and we only look for that value in the label?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added fix for priority to value instead of true/false

vjoshi3 added 3 commits July 26, 2022 22:00
Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com>
Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com>
Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com>
@vrushalijoshi vrushalijoshi force-pushed the Add-priority-to-gtp-processing branch from 8442430 to 78b5e4d Compare July 27, 2022 05:00
Copy link
Contributor

@aattuluri aattuluri left a comment

Choose a reason for hiding this comment

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

lgtm

@vrushalijoshi vrushalijoshi merged commit 1bffeaf into master Jul 27, 2022
@vrushalijoshi vrushalijoshi deleted the Add-priority-to-gtp-processing branch July 27, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants