Add openshift-tekton-resources integration #1892

rporres · 2021-10-04T10:44:47Z

https://issues.redhat.com/browse/APPSRE-3389
Design doc: https://gitlab.cee.redhat.com/service/app-interface/-/blob/master/docs/app-sre/design-docs/openshift-tekton-resources-integration.md

This work has been split in two

The new integration in this PR
The openshift-saas-deploy bits in Openshift-tekton-resources: the saasherder part #1998

We will first merge this one, verify that everything goes well and then merge the other.

Related schema changes

See app-sre/qontract-schemas#9

Current testing includes:

Creation/modification/deletion of Pipelines and Tasks
Coexistence with current coexistence of openshift-resources managed objects
Dry-run creation of openshit-saas-deploy related PipelineRuns
Do not create PipelineRuns if no correspondent openshift-saas-deploy objects do not exist

Current testing does not include

Creation of PipelineRuns and check that deployments actually work, since it needs access to an up to date qontract-server

Request and limits defaults

The variability is always in the openshift-saas-deploy step task. The rest consume a very low amount of resources. A minimum of 20Mb of memory has been assigned to avoid pod creation issues.

I have gathered a fair amount of openshift-saas-deploy metric pods examples that have been used to generate defaults. The outliers will need to be configured at a saas file level. Find examples in the related JIRA issue

Method documentation

Since the new integration uses type hints, I haven't added parameter related documentation of the methods. Let me know if that's a problem and I will add it.

Signed-off-by: Rafa Porres Molina rporresm@redhat.com

EDIT: Split PR in two.

rporres · 2021-10-04T19:27:46Z

package_data does not support recursive globs: pypa/setuptools#1806

Piojo · 2021-10-05T17:31:24Z

Nothing major stands out, but can we add some unit tests? The one thing that will get in your way is the fetching of settings in __init__ - you can mock away the entire constructor with testslide if that's problematic.

rporres · 2021-10-05T18:29:03Z

do you mean more tests apart to those that are in reconcile/test/test_openshift_tekton_resources.py?

My main worry here is how the dependency with openshift_resources_base.py is not tested. If I'm honest I don't have yet an idea on how to test that module, but it is in my personal roadmap. Since it is used by a few integrations, it makes sense to add them but I think it is a bit out of the scope of this particular MR. I can create a JIRA to track it.

Piojo

Sorry, I had forgotten to press "submit" on my comments. :|

reconcile/openshift_resources_base.py

reconcile/openshift_saas_deploy_trigger_base.py

reconcile/openshift_tekton_resources.py

Piojo · 2021-10-05T21:06:23Z

My main worry here is how the dependency with openshift_resources_base.py

While ob.realize_data isn't tested, tests are coming for init_specs_to_fetch in #1868 - or whatever incarnation we choose for it.

We could use some unit tests around _build_tkn_namespaces, in particular confirm that we skip non-tekton providers and that the resourceNames for pipelines and tasks are introduced in the right locations. I see myself screwing that up.

If you open a ticket for that I'm satisfied.

maorfr · 2021-10-06T09:08:50Z

as discussed, perhaps it would be more consistent to use openshift_base instead of openshift_resources_base (which is for integrations getting their data from the openshiftResources section).

rporres · 2021-10-06T10:11:13Z

ok with not using openshift_resources_base functions.
also ok with moving templates and template settings into its own schema

I'll get back to https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/26409

reconcile/openshift_resources_base.py

reconcile/openshift_saas_deploy_trigger_base.py

Piojo

You've addressed all comments except for a trivial one around type hints. Approving to unblock you ASAP.

rporres · 2021-10-25T17:59:44Z

Thx @Piojo. This went back to development due to #1892 (comment) (whenever I have time for it)

reconcile/openshift_tekton_resources.py

rporres · 2021-11-03T19:16:20Z

@maorfr This is ready for a preliminary review. Style wise things to fix are:

~~Missing tests~~ fixed in aeb28ab
~~The section that builds the desired state is too big, I should try to refactor in smaller pieces~~ Fixed in 5e253e7
~~No exception handling and no custom exceptions~~ fixed in 7eac1c3
~~No type hints~~ Fixed in 58c3f4f
~~I may review the idea of making a runner class as it is not giving much. I'll see when I write tests again.~~ Fixed in a49d55c

~~Functionally wise I will need to take a look to the schema to make sure we don't allow for names that are too long~~ Fixed in app-sre/qontract-schemas@bde4e6e and 0342941

Related schema changes in: app-sre/qontract-schemas#9

Piojo

Can we have some unit tests at least for the auxiliary methods in the openshift_tekton_resources integration?

reconcile/openshift_tekton_resources.py

rporres · 2021-11-09T13:12:25Z

Thx for the review, @Piojo . It was still marked as WIP as tests are missing. I will add them.

rporres · 2021-11-11T13:48:42Z

Tests added, this is ready for review now.

rporres · 2021-11-11T15:13:51Z

conflicts resolved

reconcile/test/test_openshift_tekton_resources.py

reconcile/openshift_tekton_resources.py

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres · 2021-11-15T19:19:59Z

Removed openshift-saas-deploy saasherder stuff in 613b835 and created a new PR in #1998

We have divided the work for openshift-tekton-resources in two parts: 1) The new integration in app-sre#1892 2) The saasherder bits here. They include logic to make sure that names for the resources created by the new integration are correctly referenced from openshift-saas-deploy integrations and dealing with a race condition on the new objects not yet existing (a recently created saas file). This is part of APPSRE-3389 Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

We have divided the work for openshift-tekton-resources in two parts: 1) The new integration in #1892 2) The saasherder bits here. They include logic to make sure that names for the resources created by the new integration are correctly referenced from openshift-saas-deploy integrations and dealing with a race condition on the new objects not yet existing (a recently created saas file). This is part of APPSRE-3389 Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres force-pushed the openshift-tekton-resources branch from 1b0dcdb to b8c1a31 Compare October 4, 2021 19:24

rporres force-pushed the openshift-tekton-resources branch from b8c1a31 to 2d67a88 Compare October 5, 2021 15:27

Piojo mentioned this pull request Oct 5, 2021

[openshift_resource] raise KeyError when adding an item #1880

Merged

Piojo reviewed Oct 5, 2021

View reviewed changes

Piojo reviewed Oct 6, 2021

View reviewed changes

reconcile/openshift_resources_base.py Outdated Show resolved Hide resolved

reconcile/openshift_saas_deploy_trigger_base.py Outdated Show resolved Hide resolved

Piojo approved these changes Oct 25, 2021

View reviewed changes

rporres changed the title ~~Add openshift-tekton-resources integration~~ WIP: Add openshift-tekton-resources integration Oct 25, 2021

maorfr reviewed Nov 3, 2021

View reviewed changes

reconcile/openshift_tekton_resources.py Outdated Show resolved Hide resolved

rporres force-pushed the openshift-tekton-resources branch from 19f436e to ea5a910 Compare November 3, 2021 19:07

rporres force-pushed the openshift-tekton-resources branch from 58c3f4f to 7eac1c3 Compare November 8, 2021 22:44

Piojo reviewed Nov 9, 2021

View reviewed changes

rporres force-pushed the openshift-tekton-resources branch from 56cdbfb to aad5bfe Compare November 9, 2021 12:45

rporres force-pushed the openshift-tekton-resources branch from 1001f6a to aeb28ab Compare November 11, 2021 13:35

rporres changed the title ~~WIP: Add openshift-tekton-resources integration~~ Add openshift-tekton-resources integration Nov 11, 2021

rporres force-pushed the openshift-tekton-resources branch 2 times, most recently from db3e15f to b72821f Compare November 11, 2021 15:12

Piojo approved these changes Nov 15, 2021

View reviewed changes

reconcile/test/test_openshift_tekton_resources.py Outdated Show resolved Hide resolved

reconcile/test/test_openshift_tekton_resources.py Outdated Show resolved Hide resolved

reconcile/test/test_openshift_tekton_resources.py Show resolved Hide resolved

maorfr reviewed Nov 15, 2021

View reviewed changes

reconcile/openshift_tekton_resources.py Outdated Show resolved Hide resolved

maorfr reviewed Nov 15, 2021

View reviewed changes

reconcile/openshift_tekton_resources.py Show resolved Hide resolved

rporres added 17 commits November 15, 2021 19:46

fix error conditions in unknown tasks

b3cf5b6

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

remove unused variable

d5e6454

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Added comments and remove unneeded structure

861dfc6

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

refactor run function into smaller chunks

e7b4f97

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Limit the size of the objects prefix

e0c3813

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

It didn't make sense to have a runner class

0a0b362

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

add type hints

eb4cda3

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Add custom exceptions

b861199

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Make linter happy

ddc3821

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

nicer exception names

73bdbdc

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Fixed deploy resources

2bf5481

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Fix very similar variables

2d038a5

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Tests

962e88a

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

no need to cast dict keys to set

0d5dabd

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

used enum from saasherder

0a796de

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

better fetch_saas_files test

e21b597

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Add test regarding task and pipeline names

5e72a88

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres force-pushed the openshift-tekton-resources branch from eb7b4e0 to ff63766 Compare November 15, 2021 18:46

rporres mentioned this pull request Nov 15, 2021

Openshift-tekton-resources: the saasherder part #1998

Merged

Removed saasherder stuff

613b835

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres force-pushed the openshift-tekton-resources branch from ff63766 to 613b835 Compare November 15, 2021 18:58

rporres merged commit 05471e3 into app-sre:master Nov 16, 2021

rporres deleted the openshift-tekton-resources branch November 16, 2021 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add openshift-tekton-resources integration #1892

Add openshift-tekton-resources integration #1892

rporres commented Oct 4, 2021 •

edited

rporres commented Oct 4, 2021 •

edited

Piojo commented Oct 5, 2021

rporres commented Oct 5, 2021

Piojo left a comment

Piojo commented Oct 5, 2021 •

edited

maorfr commented Oct 6, 2021

rporres commented Oct 6, 2021 •

edited

Piojo left a comment

rporres commented Oct 25, 2021 •

edited

rporres commented Nov 3, 2021 •

edited

Piojo left a comment

rporres commented Nov 9, 2021

rporres commented Nov 11, 2021

rporres commented Nov 11, 2021

rporres commented Nov 15, 2021

Add openshift-tekton-resources integration #1892

Add openshift-tekton-resources integration #1892

Conversation

rporres commented Oct 4, 2021 • edited

Related schema changes

Current testing includes:

Current testing does not include

Request and limits defaults

Method documentation

rporres commented Oct 4, 2021 • edited

Piojo commented Oct 5, 2021

rporres commented Oct 5, 2021

Piojo left a comment

Choose a reason for hiding this comment

Piojo commented Oct 5, 2021 • edited

maorfr commented Oct 6, 2021

rporres commented Oct 6, 2021 • edited

Piojo left a comment

Choose a reason for hiding this comment

rporres commented Oct 25, 2021 • edited

rporres commented Nov 3, 2021 • edited

Piojo left a comment

Choose a reason for hiding this comment

rporres commented Nov 9, 2021

rporres commented Nov 11, 2021

rporres commented Nov 11, 2021

rporres commented Nov 15, 2021

rporres commented Oct 4, 2021 •

edited

rporres commented Oct 4, 2021 •

edited

Piojo commented Oct 5, 2021 •

edited

rporres commented Oct 6, 2021 •

edited

rporres commented Oct 25, 2021 •

edited

rporres commented Nov 3, 2021 •

edited