Example on how to add a feature

This is an overview of how we added a feature:

To make auto-upgrading on the nodes an option.

Auto-upgrades are configured by nodeup. nodeup is driven by the nodeup model (which is at upup/models/nodeup/ )

Inside the nodeup model there are folders which serve three roles:

1) A folder with a well-known name means that items under that folder are treated as items of that type:

  • files
  • packages
  • services

2) A folder starting with an underscore is a tag: nodeup will only descend into that folder if a tag withthe same name is configured.

3) Remaining folders are just structural, for organization.

So auto-upgrades are currently always enabled, so the folder auto-upgrades configures them.

To make auto-upgrades option, we will rename it to a "tag" folder (_automatic_upgrades), and then plumb throughthe tag. The rename is a simple file rename.

Passing the _automatic_upgrades tag to nodeup

Tags reach nodeup from the NodeUpConfig. And this is in turn populated by the RenderNodeUpConfig template function,in apply_cluster.go.

(RenderNodeUpConfig is called inline from the instance startup script on AWS, in a heredoc. On GCE,it is rendered into its own resource, because GCE supports multiple resources for an instance)

If you look at the code for RenderNodeUpConfig, you can see that it in turn gets the tags by calling buildNodeupTags.

We want to make this optional, and it doesn't really make sense to have automatic upgrades at the instance group level:either you trust upgrades or you don't. At least that's a working theory; if we need to go the other way later we caneasily use the cluster value as the default.

So we need to add a field to ClusterSpec:

  1.     // UpdatePolicy determines the policy for applying upgrades automatically.
  2.     // Valid values:
  3.     //   'external' do not apply upgrades
  4.     //   missing: default policy (currently OS security upgrades that do not require a reboot)
  5.     UpdatePolicy *string `json:"updatePolicy,omitempty"`

A few things to note here:

  • We could probably use a boolean for today's needs, but we want to leave some flexibility, so we use a string.

  • We use a *string instead of a string so we can know if the value is actually set. This is less importantfor strings than it is for booleans, where false can be very different from unset.

  • We only define the value we care about for no - external to disable upgrades. We could probably define anactual value for enabled upgrades, but it isn't yet clear what that policy should be or what it should be called,so we leave the nil value as meaning "default policy, whatever it may be in future".

So, we just need to check if UpdatePolicy is not nil and == external; we add the tag automatic_upgrades,which enabled automatic upgrades, only if that is _not the case!

Validation

We should add some validation that the value entered is valid. We only accept nil or external right now.

Validation is done in validation.go, and is fairly simple - we just return an error if something is not valid:

  1.     // UpdatePolicy
  2.     if c.Spec.UpdatePolicy != nil {
  3.         switch (*c.Spec.UpdatePolicy) {
  4.         case UpdatePolicyExternal:
  5.             // Valid
  6.         default:
  7.             return fmt.Errorf("unrecognized value for UpdatePolicy: %v", *c.Spec.UpdatePolicy)
  8.         }
  9.     }

Tests

Prior to testing this for real, it can be handy to write a few unit tests.

We should test that validation works as we expect (in validation_test.go):

  1. func TestValidateFull_UpdatePolicy_Valid(t *testing.T) {
  2.     c := buildDefaultCluster(t)
  3.     c.Spec.UpdatePolicy = fi.String(api.UpdatePolicyExternal)
  4.     expectNoErrorFromValidate(t, c)
  5. }
  6.  
  7. func TestValidateFull_UpdatePolicy_Invalid(t *testing.T) {
  8.     c := buildDefaultCluster(t)
  9.     c.Spec.UpdatePolicy = fi.String("not-a-real-value")
  10.     expectErrorFromValidate(t, c, "UpdatePolicy")
  11. }

And we should test the nodeup tag building:

  1. func TestBuildTags_UpdatePolicy_Nil(t *testing.T) {
  2.     c := &api.Cluster{
  3.         Spec: api.ClusterSpec{
  4.             UpdatePolicy: nil,
  5.         },
  6.     }
  7.  
  8.     tags, err := buildCloudupTags(c)
  9.     if err != nil {
  10.         t.Fatalf("buildCloudupTags error: %v", err)
  11.     }
  12.  
  13.     nodeUpTags, err := buildNodeupTags(api.InstanceGroupRoleNode, c, tags)
  14.     if err != nil {
  15.         t.Fatalf("buildNodeupTags error: %v", err)
  16.     }
  17.  
  18.     if !stringSliceContains(nodeUpTags, "_automatic_upgrades") {
  19.         t.Fatalf("nodeUpTag _automatic_upgrades not found")
  20.     }
  21. }
  22.  
  23. func TestBuildTags_UpdatePolicy_External(t *testing.T) {
  24.     c := &api.Cluster{
  25.         Spec: api.ClusterSpec{
  26.             UpdatePolicy: fi.String("external"),
  27.         },
  28.     }
  29.  
  30.     tags, err := buildCloudupTags(c)
  31.     if err != nil {
  32.         t.Fatalf("buildCloudupTags error: %v", err)
  33.     }
  34.  
  35.     nodeUpTags, err := buildNodeupTags(api.InstanceGroupRoleNode, c, tags)
  36.     if err != nil {
  37.         t.Fatalf("buildNodeupTags error: %v", err)
  38.     }
  39.  
  40.     if stringSliceContains(nodeUpTags, "_automatic_upgrades") {
  41.         t.Fatalf("nodeUpTag _automatic_upgrades found unexpectedly")
  42.     }
  43. }

Documentation

Add some documentation on your new field:

  1. ## UpdatePolicy
  2.  
  3. Cluster.Spec.UpdatePolicy
  4.  
  5. Values:
  6.  
  7. * `external` do not enable automatic software updates
  8.  
  9. * unset means to use the default policy, which is currently to apply OS security updates unless they require a reboot

Additionally, consider adding documentation of your new feature to the docs in /docs. If your feature touches configuration options in config or cluster.spec, document them in cluster_spec.md.

Testing

You can make and run kops locally. But nodeup is pulled from an S3 bucket.

To rapidly test a nodeup change, you can build it, scp it to a running machine, andrun it over SSH with the output viewable locally:

make push-aws-run TARGET=admin@<publicip>

For more complete testing though, you will likely want to do a private build ofnodeup and launch a cluster from scratch.

To do this, you can repoint the nodeup source url by setting the NODEUP_URL env var,and then push nodeup using:

  1. export S3_BUCKET_NAME=<yourbucketname>
  2. make kops-install dev-upload UPLOAD_DEST=s3://${S3_BUCKET_NAME}
  3.  
  4. KOPS_VERSION=`bazel run //cmd/kops version -- --short`
  5. export KOPS_BASE_URL=https://${S3_BUCKET_NAME}.s3.amazonaws.com/kops/${KOPS_VERSION}/
  6. kops create cluster <clustername> --zones us-east-1b
  7. ...

Using the feature

Users would simply kops edit cluster, and add a value like:

  1.   spec:
  2.     updatePolicy: external

Then kops update cluster —yes would create the new NodeUpConfig, which is included in the instance startup scriptand thus requires a new LaunchConfiguration, and thus a kops rolling update. We're working on changing settingswithout requiring a reboot, but likely for this particular setting it isn't the sort of thing you need to changevery often.

Other steps

  • We could also create a CLI flag on create cluster. This doesn't seem worth it in this case; this is a relatively advanced optionfor people that already have an external software update mechanism. All the flag would do is save the default.