-
Notifications
You must be signed in to change notification settings - Fork 704
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
feat(deployments): allows for custom sizing for kubernetes distrbuted deployments in halconfig #724
Conversation
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.KubernetesVolumeMount; | ||
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.KubernetesVolumeSource; | ||
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.KubernetesVolumeSourceType; | ||
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omawhite I think IntelliJ did this. We probably want to change it back to list all the packages explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there's some setting to keep those expanded by default
// This is super-goofy. What can we do differently for this? Anything in the config parsing logic? | ||
default String stringOrNull(Object value) { | ||
return value != null ? String.valueOf(value) : null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lwander We'd be interested in your thoughts on this little nugget. The issue arises when the size specification is in form with only numeric characters: "1"
as opposed to "1000m"
or "1Gi"
.
We used String.valueOf
to ensure that those cases were parsed out of the config as strings. That, in turn, caused problems when a value was unset (null), because String.valueOf(null)
evaluates to "null"
as a string, which is not what we want to be in place when something isn't set.
Our tactical solution was this stringOrNull
method, which just feels weird. Is there something more framework-y in the wider halyard code base that we might leverage for this? Or a natural util-ish place we could stick this little method instead of implementing it twice in the deployment code? We also considered putting it in the CustomSizing class and calling that from both places where it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed this is a little funky - but this is also the first time where the type isn't explicit in the config. Would it be possible to make it clear that component sizing is mapping string -> string -> string
? Or is the fear that this is too restrictive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were concerned about being too restrictive, but at this point it probably makes sense to explicitly make it all maps with strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that is missing is restricting the java heap size. @greenkiwi pointed me at this great flag to size your heap to that of the docker container's allocated memory:
ENV JAVA_DOCKER_MEMORY_OPTS '-XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap -XX:MaxRAMFraction=1 -XshowSettings:vm'
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.KubernetesVolumeMount; | ||
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.KubernetesVolumeSource; | ||
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.KubernetesVolumeSourceType; | ||
import com.netflix.spinnaker.clouddriver.kubernetes.deploy.description.servergroup.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there's some setting to keep those expanded by default
*/ | ||
@Data | ||
@EqualsAndHashCode(callSuper = false) | ||
public class CustomSizing implements Map<String, Map> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intent here just to provide a type alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh. Not really.
Initially this class had some structure to it, because it assumed the kubernetes "requests" and "limits" nomenclature. We also extended Node initially, and then removed that after we realized we had nothing to validate because there's no CLI interaction with this data (and we couldn't think of any reasonable validation that wasn't k8s-specific). By the time we removed those things, this had nothing left but the Map implementation; you were the first to realize this. :-)
But now, we've given this class a new reason to exist: it holds our goofy little stringOrNull
method, so that it doesn't have to be duplicated in the manual and orca deployment flows.
(We abandoned the idea of explicitly making this a three-layer map with strings at its leaf nodes. No implementation felt clean, and we also realized that when we add replicaSetSize to it later this week, that will be an integer, at the top tier of the map. So we'd be breaking that constraint ourselves.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm it might still be valuable to validate even if there is no CLI command to generate entries here - but maybe not.
Sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd be wide open to it if you can guide us on what to validate in a way that doesn't assume a specific cloud provider, or on a way to detect that we're deploying to k8s and only then do our k8s-specific validations. I would definitely feel better if we validated some basics in the k8s case.
On a later PR, of course. :-)
@@ -373,14 +359,7 @@ default KubernetesContainerDescription buildContainer(String name, ServiceSettin | |||
readinessProbe.setHandler(handler); | |||
container.setReadinessProbe(readinessProbe); | |||
|
|||
/* TODO(lwander) this needs work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You guys rock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-)
To be completely honest, we only moved your TODO into the applyCustomSize method. Because we didn't implement handling of the SMALL/MEDIUM/LARGE specifiers, we kept your placeholder in its new home, below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh I saw that after
// This is super-goofy. What can we do differently for this? Anything in the config parsing logic? | ||
default String stringOrNull(Object value) { | ||
return value != null ? String.valueOf(value) : null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed this is a little funky - but this is also the first time where the type isn't explicit in the config. Would it be possible to make it clear that component sizing is mapping string -> string -> string
? Or is the fear that this is too restrictive?
@lwander should we be append the JAVA_OPTS in this code, or should we be instructing people that they should introduce that in to their service-settings files for individual services? |
I think it'd be nice to append this in this code for the k8s deployments. They can always be overridden using custom service settings. |
Hey @lwander in reference to this environment variable, we noticed that the change wasn't added until java version 8u131, but it appears that the spinnaker components are using 8u111.
|
… deployments in halconfig
be91f68
to
364d482
Compare
squashed commits |
Good catch @omawhite on the java version. |
I'm ready to merge when you guys feel ready (or you can go ahead @edwinavalos) |
see spinnaker issue 683