[DEVHAS-53] Use a Kubernetes Job to Generate GitOps Resources #256

johnmcollier · 2023-02-02T20:01:41Z

What does this PR do?:

Note: This PR supersedes #231 and #241

This PR updates HAS to use a Kubernetes Job by default to generate GitOps resources.

In all the following is done:

By default now in HAS, when GitOps resources need to be generated (either base or overlays) during the reconciliation of a Component/SEB, HAS will launch a Kubernetes Job that will execu
- If the Job fails, HAS will attempt to retrieve the job's logs and report back the error message in the component/seb status. If logs cannot be retrieved, or the job never launched, a separate error message is reported instead
- The Job will be considered "failed" after a 5 minute timeout, or 5 successive failures.
If HAS is launched with the DO_LOCAL_GITOPS_GEN environment variable, HAS will instead generate the GitOps resources locally, instead of using a job
Additionally, if HAS is launched with the ALLOW_LOCAL_GITOPS_GEN environment variable, any Component or SEB with the allowLocalGitOpsGen annotation set to true, will have its GitOps resources generated locally
The gitops/ package and subpackages have been moved to the new gitops-generator/ submodule.
Testing:
- Tests added for the new gitops-generator/ submodule
- GitHub actions to build and test the new module, along with pushing its image up to quay.io/redhat-appstudio/gitops-generator
- Controller tests updated to test both generation-locally, and generation-by-job.

Which issue(s)/story(ies) does this PR fixes:

https://issues.redhat.com/projects/DEVHAS/issues/DEVHAS-53?filter=allopenissues

PR acceptance criteria:

Unit/Functional tests
Documentation
Client Impact

How to test changes / Special notes to the reviewer:

Signed-off-by: John Collier <jcollier@redhat.com>

…e into newjobchanges Signed-off-by: John Collier <jcollier@redhat.com>

Signed-off-by: John Collier <jcollier@redhat.com>

…e into newjobchanges Signed-off-by: John Collier <jcollier@redhat.com>

Signed-off-by: John Collier <jcollier@redhat.com>

…e-system Signed-off-by: John Collier <jcollier@redhat.com>

…tart up Signed-off-by: John Collier <jcollier@redhat.com>

Signed-off-by: John Collier <jcollier@redhat.com>

openshift-ci · 2023-02-02T20:01:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: johnmcollier

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: John Collier <jcollier@redhat.com>

maysunfaisal · 2023-02-03T20:54:54Z

config/manager/kustomization.yaml

@@ -29,5 +29,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 images:
 - name: controller
-  newName: quay.io/redhat-appstudio/application-service
-  newTag: next
+  newName: quay.io/jcollier/application-service


maysunfaisal · 2023-02-03T20:58:02Z

gitops-generator/go.mod

+	github.com/openshift-pipelines/pipelines-as-code v0.0.0-20220622161720-2a6007e17200
+	github.com/openshift/api v0.0.0-20210503193030-25175d9d392d
+	github.com/redhat-appstudio/application-api v0.0.0-20221205185405-03f73a06d978
+	github.com/redhat-developer/gitops-generator v0.0.0-20230113152345-19efcd5ec104


probably pull the latest here

maysunfaisal · 2023-02-03T20:59:13Z

gitops-generator/pkg/gitops/generate.go

@@ -40,11 +39,11 @@ func GenerateTektonBuild(outputPath string, component appstudiov1alpha1.Componen
 		tektonResourcesDirName := ".tekton"

 		if err := GenerateBuild(appFs, filepath.Join(componentPath, tektonResourcesDirName), component, gitopsConfig); err != nil {
-			return util.SanitizeErrorMessage(fmt.Errorf("failed to generate tekton build in %q for component %q: %s", componentPath, componentName, err))
+			return fmt.Errorf("failed to generate tekton build in %q for component %q: %s", componentPath, componentName, err)


how come this is removed

That is... a good question. I think it's a holdover from earlier testing. This should be reverted

maysunfaisal · 2023-02-07T16:48:59Z

controllers/component_controller.go

-		return err
-	}
+	// Determine if we're using a Kubernetes job for gitops generation, or generating locally
+	localGitopsGen := (r.AllowLocalGitopsGen && component.Annotations["allowLocalGitopsGen"] == "true") || (r.DoLocalGitOpsGen)


If AllowLocalGitopsGen allows certain resources to generate gitops locally then how come we need r.AllowLocalGitopsGen? By this if statement the controller needs r.AllowLocalGitopsGen to be true always irrespective of the annotation. Am I wrong or something? 🤔

Because I didn't want to always allow local GitOps generation, only if we explicitly chose to enable it.

TBH, this is primarily for testing, since suite_test.go sets the fields for Reconcilers, so we can't toggle DoLocalGitopsGen between false/true fort the controller tests

maysunfaisal · 2023-02-07T16:50:13Z

gitops-generator/pkg/generate/generate.go

+
+	deployAssociatedComponents, err := devfileParser.GetDeployComponents(compDevfileData)
+	if err != nil {
+		//log.Error(err, "unable to get deploy components")


comments here in these files

controllers/component_controller.go

maysunfaisal · 2023-02-07T16:59:24Z

controllers/component_controller.go

+		jobNamespace := r.GitOpsJobNamespace
+		if jobNamespace == "" {
+			jobNamespace = component.Namespace
+		}


Why do we create job in the application service namespace as default?

The GitOps secret & service account are already present in the HAS namespace + avoids using up user quota for resources in the user namespace

maysunfaisal · 2023-02-07T17:01:30Z

pkg/gitopsjob/job.go

+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var gitopsJobImage = "quay.io/redhat-appstudio/gitops-generator:latest"


nit: I'd declare this as a const

Yeah, oops. Agreed.

Is it wise to pull in the latest? what if there are breaking changes from the repo? We should probably consider having releases on the gitops-generator repo and pulling in image versions that we know are compatible/tested with HAS eventually

We need to start doing releases in gitops-generator repo.. But if we dont pull in the latest here, we are going to get some of the errors back we faced in infra-deploy PR 1291!

yes, agreed for now but something to think about once we stabilize

…e into gitopsgenfinal

maysunfaisal · 2023-02-07T17:38:05Z

pkg/gitopsjob/job.go

+}
+
+func GetPodLogs(ctx context.Context, client ctrlclient.Client, clientset kubernetes.Interface, jobName string, jobNamespace string) (string, error) {
+	jobPodList, err := clientset.CoreV1().Pods(jobNamespace).List(ctx, v1.ListOptions{LabelSelector: "job-name=" + jobName})


curious as to why a clientset is being used here, rather than a call with the client, something like:

err = client.List(ctx, jobPodList, client.InNamespace(jobNamespace), client.MatchingLabels{label: "job-name=" + jobName})

No reason in particular. Figured since I had to use the clientset to get the logs, I might as well be consistent and use it to get the pods too.

pkg/gitopsjob/job.go

maysunfaisal · 2023-02-07T17:45:27Z

pkg/gitopsjob/job.go

+		select {
+		case <-timeout:
+			stay = false
+		default:


do we need a default here if its empty

Yes, otherwise go linting complains about using it with a channel, but this was the most straightforward approach

maysunfaisal · 2023-02-07T17:48:29Z

controllers/component_controller.go

+		// Wait for the job to succeed, error out if the 5 min timeout is reached
+		err = gitopsjob.WaitForJob(log, context.Background(), r.Client, r.GitOpsJobClientSet, jobName, jobNamespace, 5*time.Minute)
+		if err != nil {
+			return r.CleanUpJobAndReturn(log, jobName, jobNamespace, err)


if we return like return r.CleanUpJobAndReturn() then the gitopsjob.WaitForJob() err will be lost

CleanUpJobAndReturn explicitly only logs the error from the deletion, and we return the original error from WaitForJob.

func (r *ComponentReconciler) CleanUpJobAndReturn(log logr.Logger, jobName, jobNamespace string, err error) error { delErr := gitopsjob.DeleteJob(context.Background(), r.Client, jobName, jobNamespace) if delErr != nil { log.Error(err, "unable to delete gitops-generation job") } return err }

It's a little clunky but should do the trick

controllers/component_controller.go

maysunfaisal · 2023-02-07T17:51:42Z

controllers/component_controller.go

@@ -528,8 +543,7 @@ func (r *ComponentReconciler) generateGitops(ctx context.Context, req ctrl.Reque
 	}
 	component.Status.GitOps.CommitID = commitID

-	// Remove the temp folder that was created
-	return r.AppFS.RemoveAll(tempDir)


you need to make this call in GenerateGitopsBase() otherwise we will have a host of temp dirs for every reconcile

maysunfaisal · 2023-02-07T17:53:25Z

controllers/suite_test.go

@@ -154,5 +163,9 @@ var _ = AfterSuite(func() {
 	cancel()
 	By("tearing down the test environment")
 	err := testEnv.Stop()
+	if err != nil {
+		time.Sleep(4 * time.Second)


🤔 not sure why

Found on a GitHub issue I can no longer find. Without the sleep the tests will sometimes fail to stop at the end of execution.

kubernetes-sigs/controller-runtime#1571

maysunfaisal · 2023-02-07T18:19:12Z

controllers/applicationsnapshotenvironmentbinding_controller.go

-		r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
-		return ctrl.Result{}, err
-	}
+	// If the Job succeeds, delete it


no delete happening

That's awkward. Oops

maysunfaisal · 2023-02-07T18:20:23Z

controllers/applicationsnapshotenvironmentbinding_controller.go

+	// AllowLocalGitopsGen allows for certain resources to generate gitops resources locally, *if* an annotation is present on the resource. Defaults to false
+	AllowLocalGitopsGen bool
+
+	GitOpsJobClientSet *kubernetes.Clientset
 }

 //+kubebuilder:rbac:groups=appstudio.redhat.com,resources=snapshotenvironmentbindings,verbs=get;list;watch;create;update;patch;delete


do we need to mention the job resource and its verbs here

Not strictly necessary since the component controller specifies them, but I'll add them here to just to be consistent.

…e into gitopsgenfinal

Co-authored-by: Maysun Faisal <31771087+maysunfaisal@users.noreply.github.com>

Signed-off-by: John Collier <jcollier@redhat.com>

openshift-ci · 2023-02-09T20:10:32Z

@johnmcollier: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	`d24a233`	link	true	`/test images`
ci/prow/application-service-e2e	`d24a233`	link	true	`/test application-service-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

johnmcollier added 21 commits January 18, 2023 15:01

Use a Kube job to generate GitOps resources

ff8af89

Signed-off-by: John Collier <jcollier@redhat.com>

Update gitops job package

1dbd04e

Signed-off-by: John Collier <jcollier@redhat.com>

Ensure job name is unique

a406b29

Signed-off-by: John Collier <jcollier@redhat.com>

Update SEB controller for Kube jobs

2057be6

Signed-off-by: John Collier <jcollier@redhat.com>

Update tests

ca4b1be

Signed-off-by: John Collier <jcollier@redhat.com>

Enable GitOps job on ASEB controller

820b8ce

Signed-off-by: John Collier <jcollier@redhat.com>

Update tests and allow local gitops generation to optionally occur

ded21c4

Signed-off-by: John Collier <jcollier@redhat.com>

Merge branch 'main' of github.com:redhat-appstudio/application-servic…

6e88a4b

…e into newjobchanges Signed-off-by: John Collier <jcollier@redhat.com>

Update tests

87503f4

Signed-off-by: John Collier <jcollier@redhat.com>

Pull in gitops-generator module into Dockerfile

5967979

Signed-off-by: John Collier <jcollier@redhat.com>

Merge branch 'main' of github.com:redhat-appstudio/application-servic…

fdf2a2c

…e into newjobchanges Signed-off-by: John Collier <jcollier@redhat.com>

Pull in gitops-generator

410e20c

Signed-off-by: John Collier <jcollier@redhat.com>

Rebase and update gitops-generator tests

20dca9b

Signed-off-by: John Collier <jcollier@redhat.com>

Fix test

3d39aa6

Signed-off-by: John Collier <jcollier@redhat.com>

Fix test

fce6aec

Signed-off-by: John Collier <jcollier@redhat.com>

Update GitHub actions

47107e1

Signed-off-by: John Collier <jcollier@redhat.com>

Make default namespace application-service and not application-servic…

8d762d4

…e-system Signed-off-by: John Collier <jcollier@redhat.com>

Set values for allowLocalGitOpsGen and doLocalGitOpsGen on operator s…

0befa60

…tart up Signed-off-by: John Collier <jcollier@redhat.com>

Set resource requests for gitops job

f4a3d50

Signed-off-by: John Collier <jcollier@redhat.com>

Finish rebasing

8c493d7

Signed-off-by: John Collier <jcollier@redhat.com>

gitops/ package cleanup

11c7dc8

Signed-off-by: John Collier <jcollier@redhat.com>

Go formatting and dockerfile fixes

a70bffa

Signed-off-by: John Collier <jcollier@redhat.com>

maysunfaisal reviewed Feb 3, 2023

View reviewed changes

maysunfaisal reviewed Feb 7, 2023

View reviewed changes

controllers/component_controller.go Show resolved Hide resolved

maysunfaisal reviewed Feb 7, 2023

View reviewed changes

Merge branch 'main' of github.com:redhat-appstudio/application-servic…

5281df0

…e into gitopsgenfinal

maysunfaisal reviewed Feb 7, 2023

View reviewed changes

pkg/gitopsjob/job.go Outdated Show resolved Hide resolved

maysunfaisal reviewed Feb 7, 2023

View reviewed changes

controllers/component_controller.go Outdated Show resolved Hide resolved

maysunfaisal reviewed Feb 7, 2023

View reviewed changes

johnmcollier and others added 4 commits February 7, 2023 14:18

Merge branch 'main' of github.com:redhat-appstudio/application-servic…

6ac1ffd

…e into gitopsgenfinal

Update pkg/gitopsjob/job.go

d45a5f3

Co-authored-by: Maysun Faisal <31771087+maysunfaisal@users.noreply.github.com>

Update controllers/component_controller.go

3b0d43d

Co-authored-by: Maysun Faisal <31771087+maysunfaisal@users.noreply.github.com>

Address review comments

d24a233

Signed-off-by: John Collier <jcollier@redhat.com>

johnmcollier mentioned this pull request Aug 4, 2023

[DEVHAS-434] Move GitOps operations to Go module #377

Closed

3 tasks

johnmcollier closed this Dec 11, 2023

[DEVHAS-53] Use a Kubernetes Job to Generate GitOps Resources #256

[DEVHAS-53] Use a Kubernetes Job to Generate GitOps Resources #256

Conversation

johnmcollier commented Feb 2, 2023 • edited Loading

What does this PR do?:

Which issue(s)/story(ies) does this PR fixes:

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

openshift-ci bot commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 9, 2023

johnmcollier commented Feb 2, 2023 •

edited

Loading

maysunfaisal Feb 14, 2023 •

edited

Loading