OpenShift support #33

knikolla · 2022-03-08T18:39:19Z

NOTE: This is built on top of #32, therefore includes all commits in it

Updated Vagrantfile to setup Microshift and openshift-acct-mgt
Created ci/run_functional_tests_openshift.sh to run functional tests on Microshift and openshift-acct-mgt.
Implemented interface for Resource Allocator.
Implemented functional testing for activate allocation.
Implemented add_openshift_resource command and OpenShift resource type.

Future improvements: quotas, renaming attributes to be more generic

Closes #46

ljmcgann · 2022-04-29T15:16:06Z

src/coldfront_plugin_openstack/management/commands/register_openstack_attributes.py

-            name='OpenStack', description='OpenStack Cloud')
+            name='OpenStack', description='OpenStack Cloud'
+        )
+        resource_models.ResourceType.objects.get_or_create(


I would break these commands into register_openstack_attributes and register_openshift_attributes. Its not so clear that you would have to run this command in order to create an openshift resource.

Agree. I renamed the command to register_cloud_attributes

ljmcgann

Looks mostly good. Some comments:

The assign_role_on_user call in activate_allocation in tasks.py needs to be removed in order for the workflow to finish. add_user_to_allocation already makes this call and the duplicate PUT request always returns 400. This doesn't seem to stop the workflow from working however.

Expiring an allocation and reactivating an allocation doesn't trigger any calls to the acct-mgt service.

- Updated Vagrantfile to setup Microshift and openshift-acct-mgt - Created ci/run_functional_tests_openshift.sh to run functional tests on Microshift and openshift-acct-mgt. - Implemented interface for Resource Allocator. - Implemented first functional test. - Implemented add_openshift_resource command and OpenShift resource type. - Implemented functional testing in OpenShift CI

This checks if deletion of a project works. Also removes unnecessary cd which didn't work on Github actions.

knikolla · 2022-06-01T14:17:18Z

@larsks @jtriley @naved001 could I please get a review on this?

larsks · 2022-06-01T16:34:15Z

ci/microshift.sh

+
+sudo docker run -d --name registry --network host registry:2
+
+sleep 30


Can we replace this with an active check of some sort? E.g., polling microshift for a successful response?

Most probably. Docker registries are REST APIs, so polling for a 200 OK might just work. For polling Microshift, rather than the registry, I'm not sure.

Maybe it would be sufficient just to wait for a successful reply from the equivalent of kubectl get ns?

I'll give it a try. Thanks!

larsks · 2022-06-01T16:37:17Z

ci/microshift.sh

+git clone https://github.com/cci-moc/openshift-acct-mgt.git ~/openshift-acct-mgt
+cd ~/openshift-acct-mgt
+sudo docker build . -t "localhost:5000/cci-moc/openshift-acct-mgt:latest"
+sudo docker push "localhost:5000/cci-moc/openshift-acct-mgt:latest"


Should this be pinned to a particular version? Otherwise changes in openshift-acct-mgr could cause CI on this repository to break. Ideally we'd simply use a tagged image from a hosted repository (docker/quay/ghcr/etc), but even if we can't do that pinning to a particular commit or tag might be a good idea.

Good question. I'd rather not, since the two systems are likely to be deployed separately, and this forces us to not break compatibility.

Here's my concern, in case I wasn't clear:

In the current state, because we're pulling straight from the main branch of openshift-acct-mgt, we could end up with failing CI here just because openshift-acct-mgt is broken. By pinning to a specific known-good version we avoid that problem.

This way we're testing the code in this repository, rather than also testing the code in a dependent repository.

Does that change your mind at all?

My mind was already changed, I was just being lazy. Ideally we would have a release process for openshift-acct-mgt and pin to a release that already has a published docker container rather than build. For now it should be enough to pin to a commit hash. I'll revise.

larsks · 2022-06-01T16:42:46Z

src/coldfront_plugin_openstack/openshift.py

+        if username and password:
+            session.auth = HTTPBasicAuth(username, password)
+        if os.environ.get('FUNCTIONAL_TESTS', '') == 'True':
+            session.verify = False


Should we have an environment variable corresponding directly to session.verify (like OPENSHIFT_{var_name}_VERIFY)? I can see situation in which people might want to run this code not during test, but also without valid certificates in place.

The variable could either be a boolean value, or a path to a certificate bundle:

verify = os.getenv(f'OPENSHIFT_{var_name}_VERIFY','true') if verify: ssl.verify = (verify.lower() == 'true') if verify.lower() in ['true', 'false'] else verify

Makes sense. I'll incorporate this.

larsks · 2022-06-01T16:54:15Z

src/coldfront_plugin_openstack/openshift.py

+            return response.json()
+        if response.status_code == 404:
+            raise NotFound(f"{response.status_code}: {response.text}")
+        elif 'does not exist' in response.text or 'not found' in response.text:


I am sad that this is necessary :)

larsks · 2022-06-01T16:58:14Z

src/coldfront_plugin_openstack/utils.py

@@ -2,6 +2,10 @@
                                              AllocationAttributeType)


+def env_safe_name(name):
+    return name.replace(' ', '_').replace('-', '_').upper()


Will we ever see other punctuation in names? That is, should this use re.replace instead and just replace all non-ascii-letters-and-digits?

I hope we don't see complex resource names, but eventually it might make sense to have more robust conversions. For now I'd rather keep it like this and I opened issue #48

joachimweyl · 2022-06-03T12:54:12Z

@knikolla it looks like all of the comments are being worked on or already resolved. What are the next steps?

- Removed unnecessary sleep timers - Added 'OPENSHIFT_{var_name}_VERIFY' switch for tls verification - Pinned openshift-acct-mgt to current version

knikolla force-pushed the feature/openshift branch 3 times, most recently from 101a900 to 9d7a5bb Compare March 9, 2022 10:48

tzumainn mentioned this pull request Mar 14, 2022

Test OpenShift/ColdFront Integration CCI-MOC/esi#201

Closed

knikolla mentioned this pull request Apr 12, 2022

[ColdFront] Rename to coldfront-plugin-cloud #40

Closed

knikolla changed the base branch from main to fix/cmd April 12, 2022 17:23

knikolla changed the base branch from fix/cmd to main April 12, 2022 17:23

ljmcgann reviewed Apr 29, 2022

View reviewed changes

knikolla force-pushed the feature/openshift branch from 9d7a5bb to d9df737 Compare May 19, 2022 18:42

knikolla changed the title ~~[WIP] OpenShift support~~ OpenShift support May 19, 2022

knikolla marked this pull request as ready for review May 19, 2022 18:43

knikolla force-pushed the feature/openshift branch 2 times, most recently from 4e9b29e to c4f4edf Compare May 19, 2022 18:48

knikolla force-pushed the feature/openshift branch 2 times, most recently from 1603f11 to 1bf5aa6 Compare May 19, 2022 19:01

Add disable_project to openshift functional test

5a0e3ab

This checks if deletion of a project works. Also removes unnecessary cd which didn't work on Github actions.

knikolla force-pushed the feature/openshift branch 2 times, most recently from ff7f830 to 19d84fc Compare May 19, 2022 19:18

Don't create virtualenv if running on GitHub

793576f

knikolla force-pushed the feature/openshift branch from 19d84fc to 793576f Compare May 19, 2022 20:12

larsks reviewed Jun 1, 2022

View reviewed changes

knikolla mentioned this pull request Jun 1, 2022

Support more complex resource names when loading credential env vars #48

Open

This was referenced Jun 3, 2022

[ColdFront/OpenShift] Implement integration testing #46

Closed

[ColdFront/OpenShift] Implement support for setting granular quotas #45

Closed

[ColdFront/OpenShift] Implement support for setting quotas via quantity #44

Closed

Small revisions based on feedback

45895c2

- Removed unnecessary sleep timers - Added 'OPENSHIFT_{var_name}_VERIFY' switch for tls verification - Pinned openshift-acct-mgt to current version

knikolla force-pushed the feature/openshift branch from cbfb462 to 45895c2 Compare June 6, 2022 14:19

knikolla merged commit 667f4de into nerc-project:main Jun 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenShift support #33

OpenShift support #33

knikolla commented Mar 8, 2022 •

edited

Loading

ljmcgann Apr 29, 2022

knikolla May 19, 2022

ljmcgann left a comment

knikolla commented Jun 1, 2022

larsks Jun 1, 2022

knikolla Jun 1, 2022

larsks Jun 1, 2022

knikolla Jun 1, 2022

larsks Jun 1, 2022

knikolla Jun 1, 2022

larsks Jun 1, 2022

knikolla Jun 1, 2022

larsks Jun 1, 2022

knikolla Jun 1, 2022

larsks Jun 1, 2022

larsks Jun 1, 2022

knikolla Jun 1, 2022

joachimweyl commented Jun 3, 2022


		sudo docker run -d --name registry --network host registry:2

		sleep 30

OpenShift support #33

OpenShift support #33

Conversation

knikolla commented Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljmcgann left a comment

Choose a reason for hiding this comment

knikolla commented Jun 1, 2022

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

joachimweyl commented Jun 3, 2022

knikolla commented Mar 8, 2022 •

edited

Loading