Clean up persist output

README.md

@@ -32,7 +32,7 @@ There are two different kinds of `QuarksJob`:
 Errands are run manually by the user. They are created by setting `trigger.strategy: manual`.
 
 After the `QuarksJob` is created, run an errand by editing and applying the
-manifest, i.e. via `k edit errand1` and change `trigger.strategy: manual` to `trigger.strategy: now`. A `kubectl patch` is also a good way to trigger this type of `QuarksJob`.
+manifest, i.e. via `kubectl edit errand1` and change `trigger.strategy: manual` to `trigger.strategy: now`. A `kubectl patch` is also a good way to trigger this type of `QuarksJob`.
 
 After completion, this value is reset to `manual`.
 

docs/quarksjob.md

@@ -1,8 +1,7 @@
 # QuarksJob
 
 1. [QuarksJob](#quarksjob)
-   1. [Description](#description)
-   2. [QuarksJob Component](#quarksjob-component)
+   1. [QuarksJob Component](#quarksjob-component)
       1. [Errand Controller](#errand-controller)
          1. [Watches](#watches-in-errand-controller)
          2. [Reconciliation](#reconciliation-in-errand-controller)
@@ -11,20 +10,11 @@
          1. [Watches](#watches-in-job-controller)
          2. [Reconciliation](#reconciliation-in-job-controller)
          3. [Highlights](#highlights-in-job-controller)
-   3. [Relationship with the BDPL component](#relationship-with-the-bdpl-component)
-   4. [QuarksJob Examples](#quarksjob-examples)
+   2. [Relationship with the BDPL component](#relationship-with-the-bdpl-component)
+   3. [QuarksJob Examples](#quarksjob-examples)
 
 ## Description
 
-An `QuarksJob` allows the developer to run jobs when something interesting happens. It also allows the developer to store the output to a file /mnt/quarks/output.json which is transformed into a `Secret` later.
-The job started by an `QuarksJob` is deleted automatically after it succeeds.
-
-There are two different kinds of `QuarksJob`:
-
-- **one-offs**: automatically runs once after it's created
-- **errands**: needs to be run manually by a user
-
-## QuarksJob Component
 
 The **QuarksJob** component is a categorization of a set of controllers, under the same group. Inside the **QuarksJob** component we have a set of 2 controllers together with one separate reconciliation loop per controller.
 
@@ -51,47 +41,6 @@ This is the controller responsible for implementing **Errands**, this will lead
 - When an `QuarksJob` instance is generated, it will create an associated Kubernetes Job.
 - The generation of new Kubernetes Jobs also serves as the trigger for the `Job Controller`, to start the Reconciliation.
 
-#### Highlights in Errand controller
-
-##### Errands
-
-- Errands are run manually by the user. They are created by setting `trigger.strategy: manual`.
-
-- After the `QuarksJob` is created, run an errand by editing and applying the
-manifest, i.e. via `kubectl edit errand1` and change `trigger.strategy: manual` to `trigger.strategy: now`. A `kubectl patch` is also a good way to trigger this type of `QuarksJob`. After completion, this value is reset to `manual`.
-
-##### Auto-Errands
-
-- One-off jobs run directly when created, just like native k8s jobs.
-
-- They are created with `trigger.strategy: once` and switch to `done` when
-finished.
-
-- If a versioned secret is referenced in the pod spec of an `QuarksJob`, the most recent
-version of that secret will be used when the batchv1.Job is created.
-
-##### Restarting on Config Change
-
-- A **one-off** `QuarksJob` can
-automatically be restarted if its environment/mounts have changed, due to a
-`configMap` or a `secret` being updated. This also works for [versioned secrets](#versioned-secrets). This requires the attribute `updateOnConfigChange` to be set to true.
-
-- Once `updateOnConfigChange` is enabled, modifying the `data` of any `ConfigMap` or `Secret` referenced by the `template` section of the job will trigger the job again.
-
-##### Versioned Secrets
-
-Versioned Secrets are a set of `Secrets`, where each of them is immutable, and contains data for one iteration. Implementation can be found in the [versionedsecretstore](https://github.com/cloudfoundry-incubator/quarks-utils/tree/master/pkg/versionedsecretstore) package.
-
-When an `QuarksJob` is configured to save to "Versioned Secrets", the controller looks for the `Secret` with the largest ordinal, adds `1` to that value, and _creates a new Secret_.
-
-Each versioned secret has the following characteristics:
-
-- its name is calculated like this: `<name>-v<ORDINAL>` e.g. `mysecret-v2`
-- it has the following labels:
-  - `quarks.cloudfoundry.org/secret-kind` with a value of `versionedSecret`
-  - `quarks.cloudfoundry.org/secret-version` with a value set to the `ordinal` of the secret
-- an annotation of `quarks.cloudfoundry.org/source-description` that contains arbitrary information about the creator of the secret
-
 ### **_Job Controller_**
 
 ![job-controller-flow](quarks_qjobjobcontroller_flow.png)

pkg/kube/controllers/quarksjob/output_persistor.go

@@ -143,26 +143,39 @@ func (po *OutputPersistor) persistContainer(
 					errorContainerChannel <- errors.Wrapf(err, "failed to convert output file %s into json for creating secret(s) %s in pod '%s/%s'", filePath, options.Name, po.namespace, po.podName)
 				}
 
+				labels := newLabels(*qJob.Spec.Output, options.AdditionalSecretLabels, container)
+
 				switch options.PersistenceMethod {
 				case qjv1a1.PersistUsingFanOut:
 					po.log.Debugf("container '%s': creating secrets with prefix '%s' from '%s'", container.Name, options.Name, filePath)
 					for key, value := range data {
-						secretName := options.FanOutName(key)
-
-						var secretData map[string]string
-						if err := json.Unmarshal([]byte(value), &secretData); err != nil {
+						name := names.SanitizeSubdomain(options.FanOutName(key))
+						var stringData map[string]string
+						if err := json.Unmarshal([]byte(value), &stringData); err != nil {
 							errorContainerChannel <- err
 						}
 
-						if err := po.createSecret(ctx, qJob, container, secretName, options.AdditionalSecretAnnotations, secretData, options.AdditionalSecretLabels, options.Versioned); err != nil {
-							errorContainerChannel <- err
+						if options.Versioned {
+							err = po.createVersionedSecret(qJob, name, labels, options.AdditionalSecretAnnotations, stringData)
+						} else {
+							err = po.createSecret(ctx, name, labels, options.AdditionalSecretAnnotations, stringData)
+						}
+						if err != nil {
+							errorContainerChannel <- errors.Wrapf(err, "failed to persist qjob '%s' output, pod '%s/%s', container '%s', using fan-out", qJob.Name, po.namespace, po.podName, container.Name)
 						}
 					}
 
 				default:
-					po.log.Debugf("container '%s': creating secret '%s' from '%s'", container.Name, options.Name, filePath)
-					if err := po.createSecret(ctx, qJob, container, options.Name, options.AdditionalSecretAnnotations, data, options.AdditionalSecretLabels, options.Versioned); err != nil {
-						errorContainerChannel <- err
+					name := names.SanitizeSubdomain(options.Name)
+					po.log.Debugf("container '%s': creating secret '%s' from '%s'", container.Name, name, filePath)
+					var err error
+					if options.Versioned {
+						err = po.createVersionedSecret(qJob, name, labels, options.AdditionalSecretAnnotations, data)
+					} else {
+						err = po.createSecret(ctx, name, labels, options.AdditionalSecretAnnotations, data)
+					}
+					if err != nil {
+						errorContainerChannel <- errors.Wrapf(err, "failed to persist qjob '%s' output, pod '%s/%s', container '%s', using one-to-one", qJob.Name, po.namespace, po.podName, container.Name)
 					}
 				}
 			}
@@ -291,71 +304,76 @@ func fileExists(filename string) bool {
 	return !info.IsDir()
 }
 
-// createSecret converts the output file into json and creates a secret for a given container
-func (po *OutputPersistor) createSecret(
-	ctx context.Context,
-	qJob *qjv1a1.QuarksJob,
-	container corev1.Container,
-	secretName string,
-	secretAnnotations map[string]string,
-	secretData map[string]string,
-	additionalSecretLabels map[string]string,
-	versioned bool,
-) error {
-	secretLabels := map[string]string{}
-	for k, v := range qJob.Spec.Output.SecretLabels {
-		secretLabels[k] = names.Sanitize(v)
+func newLabels(output qjv1a1.Output, additionalSecretLabels map[string]string, container corev1.Container) map[string]string {
+	labels := map[string]string{}
+	for k, v := range output.SecretLabels {
+		labels[k] = names.Sanitize(v)
 	}
 	for k, v := range additionalSecretLabels {
-		secretLabels[k] = names.Sanitize(v)
+		labels[k] = names.Sanitize(v)
 	}
-	secretLabels[qjv1a1.LabelPersistentSecretContainer] = names.Sanitize(container.Name)
+	labels[qjv1a1.LabelPersistentSecretContainer] = names.Sanitize(container.Name)
 	if id, ok := podutil.LookupEnv(container.Env, qjv1a1.RemoteIDKey); ok {
-		secretLabels[qjv1a1.LabelRemoteID] = id
+		labels[qjv1a1.LabelRemoteID] = id
 	}
+	return labels
+}
 
-	secretName = names.SanitizeSubdomain(secretName)
-
-	if versioned {
-		ownerName := qJob.GetName()
-		ownerID := qJob.GetUID()
-		sourceDescription := "created by quarksJob"
+func (po *OutputPersistor) createVersionedSecret(
+	qJob *qjv1a1.QuarksJob,
+	name string,
+	labels map[string]string,
+	annotations map[string]string,
+	data map[string]string,
+) error {
+	ownerName := qJob.GetName()
+	ownerID := qJob.GetUID()
+	sourceDescription := "created by quarksJob"
 
-		store := versionedsecretstore.NewClientsetVersionedSecretStore(po.clientSet)
-		err := store.Create(context.Background(), po.namespace, ownerName, ownerID, secretName, secretData, secretAnnotations, secretLabels, sourceDescription)
-		if err != nil {
-			if !versionedsecretstore.IsSecretIdenticalError(err) {
-				return errors.Wrapf(err, "could not persist qJob's '%s' output to a secret", qJob.GetNamespacedName())
-			}
-			// No-op. the latest version is identical to the one we have
-			return nil
-		}
-	} else {
-		secret := &corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      secretName,
-				Namespace: po.namespace,
-			},
+	store := versionedsecretstore.NewClientsetVersionedSecretStore(po.clientSet)
+	err := store.Create(context.Background(), po.namespace, ownerName, ownerID, name, data, annotations, labels, sourceDescription)
+	if err != nil {
+		if !versionedsecretstore.IsSecretIdenticalError(err) {
+			return errors.Wrap(err, "failed to create versioned secret")
 		}
+		// No-op. the latest version is identical to the one we have
+		return nil
+	}
+	return nil
+}
 
-		secret.StringData = secretData
-		secret.Labels = secretLabels
-		secret.Annotations = secretAnnotations
+// createSecret converts the output file into json and creates a secret for a given container
+func (po *OutputPersistor) createSecret(
+	ctx context.Context,
+	name string,
+	labels map[string]string,
+	annotations map[string]string,
+	data map[string]string,
+) error {
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: po.namespace,
+		},
+	}
 
-		_, err := po.clientSet.CoreV1().Secrets(po.namespace).Create(ctx, secret, metav1.CreateOptions{})
+	secret.StringData = data
+	secret.Labels = labels
+	secret.Annotations = annotations
 
-		if err != nil {
-			if apierrors.IsAlreadyExists(err) {
-				// If it exists update it
-				_, err = po.clientSet.CoreV1().Secrets(po.namespace).Update(ctx, secret, metav1.UpdateOptions{})
-				if err != nil {
-					return errors.Wrapf(err, "failed to update secret %s for container %s in pod '%s/%s'", secretName, container.Name, po.namespace, po.podName)
-				}
-			} else {
-				return errors.Wrapf(err, "failed to create secret %s for container %s in pod '%s/%s'", secretName, container.Name, po.namespace, po.podName)
+	_, err := po.clientSet.CoreV1().Secrets(po.namespace).Create(ctx, secret, metav1.CreateOptions{})
+
+	if err != nil {
+		if apierrors.IsAlreadyExists(err) {
+			// If it exists update it
+			_, err = po.clientSet.CoreV1().Secrets(po.namespace).Update(ctx, secret, metav1.UpdateOptions{})
+			if err != nil {
+				return errors.Wrapf(err, "failed to update secret '%s'", name)
 			}
+		} else {
+			return errors.Wrapf(err, "failed to create secret '%s'", name)
 		}
-
 	}
+
 	return nil
 }

pkg/kube/controllers/quarksjob/output_persistor_test.go

@@ -267,10 +267,15 @@ var _ = Describe("OutputPersistor", func() {
 					Expect(err).NotTo(HaveOccurred())
 					secret, _ := clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "foo-busybox", metav1.GetOptions{})
 					Expect(secret).ShouldNot(BeNil())
+					Expect(secret.StringData).To(HaveKeyWithValue("hello", "world"))
+
 					secret, _ = clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "fake-nats", metav1.GetOptions{})
 					Expect(secret).ShouldNot(BeNil())
+					Expect(secret.StringData).To(HaveKeyWithValue("hello", "world"))
+
 					secret, _ = clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "bar-nuts-v1", metav1.GetOptions{})
 					Expect(secret).ShouldNot(BeNil())
+					Expect(secret.StringData).To(HaveKeyWithValue("hello", "world"))
 				})
 			})
 
@@ -321,8 +326,17 @@ var _ = Describe("OutputPersistor", func() {
 				It("creates a secret per each key/value of the given input file", func() {
 					Expect(po.Persist(context.Background())).NotTo(HaveOccurred())
 
-					Expect(clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "link-nats-deployment-nats-nats", metav1.GetOptions{})).ShouldNot(BeNil())
-					Expect(clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "link-nats-deployment-nats-nuts", metav1.GetOptions{})).ShouldNot(BeNil())
+					secret, _ := clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "link-nats-deployment-nats-nats", metav1.GetOptions{})
+					Expect(secret).ShouldNot(BeNil())
+					Expect(secret.StringData).To(HaveKeyWithValue("nats.password", "changeme"))
+					Expect(secret.StringData).To(HaveKeyWithValue("nats.port", "1337"))
+					Expect(secret.StringData).To(HaveKeyWithValue("nats.user", "admin"))
+
+					secret, _ = clientSet.CoreV1().Secrets(namespace).Get(context.Background(), "link-nats-deployment-nats-nuts", metav1.GetOptions{})
+					Expect(secret).ShouldNot(BeNil())
+					Expect(secret.StringData).To(HaveKeyWithValue("nats.password", "chungeme"))
+					Expect(secret.StringData).To(HaveKeyWithValue("nats.port", "1337"))
+					Expect(secret.StringData).To(HaveKeyWithValue("nats.user", "udmin"))
 				})
 			})
 		})