Skip to content

Add node affinity support #1166

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

Merged
merged 23 commits into from
Dec 16, 2020
Merged

Add node affinity support #1166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f842529
Merge pull request #1 from zalando/master
sagor999 Oct 10, 2020
abc2cb4
Merge pull request #3 from zalando/master
sagor999 Nov 17, 2020
d412754
Merge pull request #4 from zalando/master
sagor999 Dec 6, 2020
abc48a6
Merge pull request #5 from zalando/master
sagor999 Dec 15, 2020
c7aadba
Adding nodeaffinity support alongside node_readiness_label
spohner May 13, 2020
4014442
Add nodeAffinity support to ConnectionPooler
adastleyatvi Jul 3, 2020
b6a83ca
Add tests for ConnectionPooler nodeAffinity
adastleyatvi Oct 10, 2020
9b53763
fix compareStatefulSet to take into account nodeAffinity
sagor999 Oct 11, 2020
b8751e8
add documentation for node affinity
sagor999 Oct 12, 2020
58c2142
add node affinity test
sagor999 Oct 12, 2020
300183f
fix print statement
sagor999 Oct 18, 2020
04be950
update codegen
sagor999 Oct 20, 2020
a3dea03
fix crd api for node affinity
sagor999 Nov 17, 2020
f032cd5
fix node affinity test
sagor999 Nov 17, 2020
fc9d5bf
fix e2e test for node affinity
sagor999 Nov 18, 2020
88743f8
replace hardcoded sleep with wait_for_pod_start in node affinity e2e
sagor999 Nov 18, 2020
a8c9b0e
remove extra affinity check, as it is already done
sagor999 Dec 6, 2020
8f05753
improve crd readability by moving Required field up
sagor999 Dec 6, 2020
03f91c3
improve node affinity e2e test
sagor999 Dec 6, 2020
867b294
add node affinity into various crds and add example into complete man…
sagor999 Dec 6, 2020
3bfa3af
fix missing type in crd
sagor999 Dec 6, 2020
8e8cd93
remove node affinity from connection pooler
sagor999 Dec 15, 2020
f5a871b
fix unit test for node affinity
sagor999 Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix e2e test for node affinity
  • Loading branch information
@sagor999
sagor999 committed Dec 15, 2020
commit fc9d5bff078d5cece23c2791d763e65490c38c10
35 changes: 16 additions & 19 deletions e2e/tests/test_e2e.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,18 +941,18 @@ def test_node_affinity(self):
current_master_node, current_replica_nodes = k8s.get_pg_nodes(cluster_label)

# label node with environment=postgres
body = {
node_label_body = {
"metadata": {
"labels": {
"environment": "postgres"
"node-affinity-test": "postgres"
}
}
}

try:
# patch current master node with the label
print('patching master node: %s', current_master_node)
k8s.api.core_v1.patch_node(current_master_node, body)
print('patching master node: {}'.format(current_master_node))
k8s.api.core_v1.patch_node(current_master_node, node_label_body)

# add node affinity to cluster
patch_node_affinity_config = {
Expand All @@ -963,7 +963,7 @@ def test_node_affinity(self):
{
"matchExpressions": [
{
"key": "environment",
"key": "node-affinity-test",
"operator": "In",
"values": [
"postgres"
Expand All @@ -976,6 +976,7 @@ def test_node_affinity(self):
}
}
}

k8s.api.custom_objects_api.patch_namespaced_custom_object(
group="acid.zalan.do",
version="v1",
Expand All @@ -987,8 +988,8 @@ def test_node_affinity(self):
# bounce replica and check if it has migrated to proper node that has a label
k8s.api.core_v1.delete_namespaced_pod("acid-minimal-cluster-1", "default")

# wait a little
time.sleep(60)
# wait for operator to apply node affinity changes
time.sleep(120)

podsList = k8s.api.core_v1.list_namespaced_pod('default', label_selector=cluster_label)
for pod in podsList.items:
Expand All @@ -999,21 +1000,14 @@ def test_node_affinity(self):
# check that pod has correct node affinity
key = pod.spec.affinity.node_affinity.required_during_scheduling_ignored_during_execution.node_selector_terms[0].match_expressions[0].key
value = pod.spec.affinity.node_affinity.required_during_scheduling_ignored_during_execution.node_selector_terms[0].match_expressions[0].values[0]
self.assertEqual("environment", key,
"Sanity check: expect node selector key to be equal to 'environment' but got {}".format(key))
self.assertEqual("node-affinity-test", key,
"Sanity check: expect node selector key to be equal to 'node-affinity-test' but got {}".format(key))
self.assertEqual("postgres", value,
"Sanity check: expect node selector value to be equal to 'postgres' but got {}".format(value))

# remove node affinity and let other tests continue normally
patch_node_affinity_config = {
patch_node_remove_affinity_config = {
"spec": {
"nodeAffinity" : {
"requiredDuringSchedulingIgnoredDuringExecution": {
"nodeSelectorTerms": [
{}
]
}
}
"nodeAffinity" : None
}
}
k8s.api.custom_objects_api.patch_namespaced_custom_object(
Expand All @@ -1022,7 +1016,10 @@ def test_node_affinity(self):
namespace="default",
plural="postgresqls",
name="acid-minimal-cluster",
body=patch_node_affinity_config)
body=patch_node_remove_affinity_config)
Copy link
Member

@FxKu FxKu Dec 1, 2020

Choose a reason for hiding this comment

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

Removing the affinity is another rolling update. Therefore, we should guarantee that everything is running when we leave the test.. Check other e2e tests like test_zz_node_readiness_label or test_zzz_taint_based_eviction how it is handled there.

Copy link
Contributor Author

@sagor999 sagor999 Dec 6, 2020

Choose a reason for hiding this comment

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

Will update!


# wait for operator to apply node affinity changes
time.sleep(120)

except timeout_decorator.TimeoutError:
print('Operator log: {}'.format(k8s.get_operator_log()))
Expand Down
166 changes: 162 additions & 4 deletions pkg/apis/acid.zalan.do/v1/crds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,87 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
},
"nodeAffinity": {
Copy link
Member

@FxKu FxKu Dec 1, 2020

Choose a reason for hiding this comment

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

gosh, that's a lot of lines. Is this validation covering all affinity options? Maybe we just cut it short with:

"nodeAffinity": {
	Type:                   "object",
	XPreserveUnknownFields: util.True(),
},

Copy link
Member

@FxKu FxKu Dec 1, 2020

Choose a reason for hiding this comment

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

at least, makes it easier to copy it to the CRD yaml manifest where this part is still missing, I think

Copy link
Contributor Author

@sagor999 sagor999 Dec 2, 2020

Choose a reason for hiding this comment

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

@FxKu for some reason, it was just kept trimming extra fields, even with XPreserveUnknownFields. That is why I had to expand it almost fully. :(

Copy link
Contributor Author

@sagor999 sagor999 Dec 6, 2020

Choose a reason for hiding this comment

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

I just tested it again, and yes, even with XPreserveUnknownFields it still trims unknown fields and completely breaks node affinity support. Btw, before Nov 17th, it used to work fine.

Copy link
Member

@FxKu FxKu Dec 15, 2020

Choose a reason for hiding this comment

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

ok, can you remove this part from the validation as it covers the anti_affinity of the pooler

Copy link
Contributor Author

@sagor999 sagor999 Dec 15, 2020

Choose a reason for hiding this comment

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

sounds good! will remove

Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
Properties: map[string]apiextv1.JSONSchemaProps{
"preferredDuringSchedulingIgnoredDuringExecution": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextv1.JSONSchemaProps{
"preference": {
Type: "object",
Properties: map[string]apiextv1.JSONSchemaProps{
"matchExpressions": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
"matchFields": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
},
},
"weight": {
Type: "integer",
Format: "int32",
},
},
Required: []string{"preference, weight"},
Copy link
Member

@FxKu FxKu Dec 1, 2020

Choose a reason for hiding this comment

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

can you also move it above Properties? I know, it's not alphabetical order, but improves readability a little.

Copy link
Contributor Author

@sagor999 sagor999 Dec 6, 2020

Choose a reason for hiding this comment

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

will do!

},
},
},
"requiredDuringSchedulingIgnoredDuringExecution": {
Type: "object",
Required: []string{"nodeSelectorTerms"},
Properties: map[string]apiextv1.JSONSchemaProps{
"nodeSelectorTerms": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextv1.JSONSchemaProps{
"matchExpressions": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
"matchFields": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
},
},
},
},
},
},
},
},
},
Expand Down Expand Up @@ -605,8 +684,87 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
},
"nodeAffinity": {
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
Properties: map[string]apiextv1.JSONSchemaProps{
"preferredDuringSchedulingIgnoredDuringExecution": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextv1.JSONSchemaProps{
"preference": {
Type: "object",
Properties: map[string]apiextv1.JSONSchemaProps{
"matchExpressions": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
"matchFields": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
},
},
"weight": {
Type: "integer",
Format: "int32",
},
},
Required: []string{"preference, weight"},
Copy link
Member

@FxKu FxKu Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

can you also move it above Properties? I know, it's not alphabetical order, but improves readability a little.

Copy link
Contributor Author

@sagor999 sagor999 Dec 6, 2020

Choose a reason for hiding this comment

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

will do!

},
},
},
"requiredDuringSchedulingIgnoredDuringExecution": {
Type: "object",
Required: []string{"nodeSelectorTerms"},
Properties: map[string]apiextv1.JSONSchemaProps{
"nodeSelectorTerms": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextv1.JSONSchemaProps{
"matchExpressions": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
"matchFields": {
Type: "array",
Items: &apiextv1.JSONSchemaPropsOrArray{
Schema: &apiextv1.JSONSchemaProps{
Type: "object",
AdditionalProperties: &apiextv1.JSONSchemaPropsOrBool{
Allows: true,
},
},
},
},
},
},
},
},
},
},
},
},
"tolerations": {
Expand Down