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

[scheduling] Moved pod affinity and anti-affinity from annotations to api fields #25319 #39478

Merged
merged 1 commit into from
Jan 12, 2017
Merged

[scheduling] Moved pod affinity and anti-affinity from annotations to api fields #25319 #39478

merged 1 commit into from
Jan 12, 2017

Conversation

rrati
Copy link

@rrati rrati commented Jan 5, 2017 •
edited
Loading

Converted pod affinity and anti-affinity from annotations to api fields

Related: #25319
Related: #34508

Release note:
Pod affinity and anti-affinity has moved from annotations to api fields in the pod spec. Pod affinity or anti-affinity that is defined in the annotations will be ignored.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 5, 2017
@rrati rrati force-pushed the pod-affinity-api-fields branch from 4b6844a to f48d135 Compare January 5, 2017 20:00
@timothysc timothysc assigned timothysc and davidopp and unassigned fejta Jan 5, 2017
@timothysc timothysc added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jan 5, 2017
@rrati rrati force-pushed the pod-affinity-api-fields branch from f48d135 to 60bcadb Compare January 6, 2017 13:45
@timothysc timothysc self-requested a review January 6, 2017 17:46
timothysc
timothysc suggested changes Jan 9, 2017
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

We should really try to eliminate the forced additions.

pkg/api/validation/validation_test.go
}]
},
Spec: api.PodSpec{
Containers: []api.Container{{Name: "ctr", Image: "image", ImagePullPolicy: "IfNotPresent"}},
Copy link
Member

Choose a reason for hiding this comment

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

So I'm guessing this section is a copy<>paste of validPodSpec? b/c it's repeated.

Copy link
Author

@rrati rrati Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed. Created a function to get around the copying of validPodSpec

pkg/api/validation/validation_test.go
},
},
Namespaces: []string{"ns"},
TopologyKey: "",
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be here if unused?

Copy link
Author

Choose a reason for hiding this comment

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

Original test has an empty value for topologyKey. I would think it shouldn't matter if it is there and empty vs not there at all. I will test and if there's no difference just remove it

pkg/api/validation/validation_test.go
},
},
Namespaces: []string{"ns"},
TopologyKey: "",
Copy link
Member

Choose a reason for hiding this comment

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

same comment re: empty string

Copy link
Author

Choose a reason for hiding this comment

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

What empty string?

pkg/api/validation/validation_test.go
},
},
Namespaces: []string{"ns"},
TopologyKey: "",
Copy link
Member

Choose a reason for hiding this comment

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

same comment re: empty strings.

Copy link
Author

Choose a reason for hiding this comment

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

Same reply. Original had it so I kept it. I will test with it removed and if same results will push update with it removed

plugin/pkg/scheduler/algorithm/priorities/balanced_resource_allocation_test.go
@@ -37,12 +37,15 @@ func TestBalancedResourceAllocation(t *testing.T) {
}
machine1Spec := v1.PodSpec{
NodeName: "machine1",
Affinity: &v1.Affinity{},
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not omit_if_empty? It seems like we're filling in blanks in a lot of locations.

We don't do that with other fields.

Copy link
Author

Choose a reason for hiding this comment

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

It is omit_empty. Issue is in the scheduler and has been addressed.

@rrati rrati force-pushed the pod-affinity-api-fields branch 2 times, most recently from be81fc6 to 6e49aba Compare January 11, 2017 15:29
@rrati
Copy link
Author

rrati commented Jan 11, 2017

@k8s-bot bazel test this

@rrati
Copy link
Author

rrati commented Jan 11, 2017

@k8s-bot unit test this

1 similar comment
@rrati
Copy link
Author

rrati commented Jan 11, 2017

@k8s-bot unit test this

timothysc
timothysc reviewed Jan 11, 2017
View reviewed changes
plugin/pkg/admission/antiaffinity/admission_test.go
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

What's with the comment blob?

Copy link
Author

Choose a reason for hiding this comment

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

missed cleanup. Removed

timothysc
timothysc reviewed Jan 11, 2017
View reviewed changes
plugin/pkg/admission/antiaffinity/admission_test.go
},
},
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

missed cleanup. Removed

timothysc
timothysc reviewed Jan 11, 2017
View reviewed changes
plugin/pkg/scheduler/algorithm/predicates/predicates_test.go
},
},
ObjectMeta: v1.ObjectMeta{Labels: podLabel},
/*
Copy link
Member

Choose a reason for hiding this comment

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

same.

Copy link
Author

Choose a reason for hiding this comment

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

Missed cleanup. Removed

Copy link
Member

Choose a reason for hiding this comment

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

still there and in a couple of other places.

@timothysc
Copy link
Member

@rrati only comments now are to remove the commented legacy blobs and get the tests fixed, then lgtm. thx for the cleanup work, it looks a lot cleaner!

@timothysc timothysc added this to the v1.6 milestone Jan 11, 2017
@rrati rrati force-pushed the pod-affinity-api-fields branch from 6e49aba to 5b3f019 Compare January 11, 2017 17:16
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017
@rrati rrati force-pushed the pod-affinity-api-fields branch from 5b3f019 to d20daf7 Compare January 11, 2017 17:59
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017
@rrati rrati force-pushed the pod-affinity-api-fields branch 3 times, most recently from 4cc39c6 to 089e057 Compare January 12, 2017 14:35
timothysc
timothysc suggested changes Jan 12, 2017
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

2 main themes..

  • Still more comment blocks.
  • Empty namespace args.

plugin/pkg/admission/antiaffinity/admission_test.go
},
},
},
Namespaces: []string{""},
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment around name-spacing. Why do we need to provide it if it's empty? nil = default namespace right.

plugin/pkg/admission/antiaffinity/admission_test.go
}]
}}`,
TopologyKey: metav1.LabelHostname,
Namespaces: []string{""},
Copy link
Member

Choose a reason for hiding this comment

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

same comment

plugin/pkg/scheduler/algorithm/priorities/interpod_affinity_test.go
},
},
},
Namespaces: []string{},
Copy link
Member

Choose a reason for hiding this comment

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

same comment.

plugin/pkg/scheduler/algorithm/priorities/interpod_affinity_test.go
"preferredDuringSchedulingIgnoredDuringExecution": [
{
"weight": 8,
/*
Copy link
Member

Choose a reason for hiding this comment

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

remove comment block

plugin/pkg/scheduler/algorithm/priorities/interpod_affinity_test.go
},
},
}
/*
Copy link
Member

Choose a reason for hiding this comment

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

remove comment block

test/e2e/scheduler_predicates.go
},
},
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

remove comment block.

test/e2e/scheduler_predicates.go
},
},
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

remove comment block.

test/e2e/scheduler_predicates.go
},
},
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

same.

test/e2e/scheduler_predicates.go
},
},
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

same.

test/e2e/scheduler_predicates.go
"operator": "In",
"values":["S2"]
},
/*
Copy link
Member

Choose a reason for hiding this comment

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

same.

@rrati rrati force-pushed the pod-affinity-api-fields branch from 089e057 to 974ac92 Compare January 12, 2017 15:56
@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit 974ac92dfb3f6fe273644772d6dab60b9feed267. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rrati
Copy link
Author

rrati commented Jan 12, 2017

@k8s-bot bazel test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2017
@timothysc timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2017
@timothysc
Copy link
Member

needs rebase and re-test run (likely from me) :-/

/lgtm

@rrati rrati force-pushed the pod-affinity-api-fields branch from 974ac92 to 6a3ad93 Compare January 12, 2017 20:05
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 39803, 39698, 39537, 39478)

@k8s-github-robot k8s-github-robot merged commit 082ce00 into kubernetes:master Jan 12, 2017
@davidopp davidopp mentioned this pull request Jan 24, 2017
Closed
This was referenced Feb 12, 2017
Closed
Merged
This was referenced Mar 16, 2017
Closed
Closed
@rrati rrati mentioned this pull request Jul 10, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothysc timothysc timothysc approved these changes

Assignees

@timothysc timothysc

@davidopp davidopp

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
7 participants
@rrati @k8s-reviewable @timothysc @k8s-ci-robot @k8s-github-robot @fejta @davidopp