Support for Subnets specification/override #454
Conversation
ellistarn
changed the title
ellistarn
changed the title
ellistarn
changed the title
ellistarn
force-pushed
the
subnets
branch
2 times, most recently
from
60887fe to
4d50be5
Compare
ellistarn
force-pushed
the
subnets
branch
4 times, most recently
from
d44104b to
9e0a438
Compare
ellistarn
changed the title
ellistarn
force-pushed
the
subnets
branch
5 times, most recently
from
b15ce16 to
0097c90
Compare
ellistarn
changed the title
ellistarn
force-pushed
the
subnets
branch
3 times, most recently
from
10add0b to
70b6f27
Compare
ellistarn
changed the title
| OperatingSystem: p.Spec.Constraints.getOperatingSystem(pod), | ||
| } | ||
| func (c *Constraints) WithLabel(key string, value string) *Constraints { | ||
| c.Labels = functional.UnionStringMaps(c.Labels, map[string]string{key: value}) |
There was a problem hiding this comment.
Could this also be done with c.Labels[key] = value?
functional.UnionStringMaps() works with "last write wins", so it seems like we overwrite an existing entry in c.labels with either versions, so it might be cleaner just to add it in.
There was a problem hiding this comment.
Oopsie, nice call!
| ProvisionerNameLabelKey, | ||
| ProvisionerNamespaceLabelKey, | ||
| ProvisionerPhaseLabel, | ||
| ProvisionerTTLKey, |
There was a problem hiding this comment.
This is an annotation, so you may not need to check this in labels.
| func() error { return f.matchesProvisioner(ctx, &pod, provisioner) }, | ||
| func() error { return f.hasSupportedSchedulingConstraints(ctx, &pod) }, | ||
| func() error { return f.toleratesTaints(ctx, &pod, provisioner) }, | ||
| func() error { return f.withValidConstraints(ctx, &pod, provisioner) }, |
There was a problem hiding this comment.
Maybe I'm missing something, but it looks like none of these 5 validation functions and their sub-functions use contexts. Do we need these contexts here?
There was a problem hiding this comment.
Knative has a logging construct (logger.FromContext(ctx).Info ...) and other amazing utilities around the ctx object (i.e. reading global configuration values from a configmap).
I had left this in as a best practice for passing contexts around. However, I think this is probably premature, and easy to add as necessary in the future, so I'll rip this out for now. Thanks!
| ExpectCreatedWithStatus(c, pod) | ||
| ExpectCreated(c, provisioner) | ||
| ExpectEventuallyReconciled(c, provisioner) | ||
| return ExpectPodExists(c, pod.GetName(), pod.GetNamespace()) |
There was a problem hiding this comment.
I like this. It looks much cleaner. I have some quick sanity checks:
- Is it possible for us to
Getthe pod before a node has been provisioned for it? If so, it's possible for this to succeed, but have the following command in tests (ExpectNodeExists) to fail undesirably. - Additionally, after using this in tests, we check to see if the node exists based on the pod's nodeName. Can we ensure the pod's nodeName will exist before we finish this function?
There was a problem hiding this comment.
As long as the reconciliation loop has executed, we're guaranteed that if a node/scheduling decisions would have been made, it's made by the time the reconcile loop is complete.
pkg/test/pods.go
Outdated
| func defaults(options PodOptions) *v1.Pod { | ||
| // Pod creates a test pod with defaults that can be overriden by PodOptions. | ||
| // Overrides are applied in order, with a last write wins semantic. | ||
| func Pod(optionss ...PodOptions) *v1.Pod { |
There was a problem hiding this comment.
nit: Is this normal? lol. Seems a little weird. Maybe options -> opts -> o?
There was a problem hiding this comment.
How about overrides.
| func (s *ProvisionerSpec) validate(ctx context.Context) (errs *apis.FieldError) { | ||
| return errs.Also( | ||
| s.Cluster.validate(ctx).ViaField("cluster"), | ||
| // This validation is on the ProvisionerSpec despire the fact that |
| func() error { return f.hasSupportedSchedulingConstraints(&pod) }, | ||
| func() error { return f.toleratesTaints(&pod, provisioner) }, | ||
| func() error { return f.hasSupportedLabels(&pod) }, | ||
| func() error { return f.isUnschedulable(ctx, &pod) }, |
There was a problem hiding this comment.
Do you need to add ctx to all these? Looks like the only one that uses it is withValidConstraints
Issue #, if available:
Description of changes:
node.k8s.aws/subnet-name: dns-compliant-namenode.k8s.aws/subnet-tag-key: kubernetes-label-compliant-tag-key-nameBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.