Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement: add additional information to environment services #3641

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

shreddedbacon
Copy link
Member

@shreddedbacon shreddedbacon commented Jan 24, 2024

General Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for inclusion in changelog

Database Migrations

  • If your PR contains a database migation, it MUST be the latest in date order alphabetically

This just extends the existing environment_services by adding the service type and containers associated with the service. This will allow better visibility of what services an environment has deployed in the API.

Some benefits of this will also make the implementation of workload resources easier, because now the API will be able to determine what service types an environment has.

Additionally, this information being available can allow for the UI to be able to generate command examples for being able to SSH to services and containers.

This will require changes in the remote-controller so that it sends the new information, this work has not been done yet, but as the API has been defined, and it contains backward compatible implementation (the setEnvironmentServices mutation still works for now, and the support in the actions-handler remains). The remote-controller is currently staging a bump to the CRD version so that we can introduce the schema changes to the CRD and not have to rely on users having to remember to update the CRDs after installation, as the new CRD version will be installed.

It would be great to get this merged so that at least the support is there for when the remote-controller gets the schema changes. This means that there will be a period where old and new controllers will be able to send their messages to core to update environment services. Just older remotes will not send the right payload to update the services with the new information.

As there are no other services in the API or Lagoon generally that could, or do, use this additional information, the delay in the remote-controller sending the information should not be seen as a blocker to this merging. It may just mean that the final removal of the deprecated mutations may be delayed longer.

Requires uselagoon/machinery#39

@shreddedbacon shreddedbacon added this to the 2.18.0 milestone Jan 24, 2024
@shreddedbacon shreddedbacon marked this pull request as ready for review January 24, 2024 21:42
@shreddedbacon shreddedbacon changed the title refactor: add additional information to environment services Change: add additional information to environment services Feb 6, 2024
@shreddedbacon shreddedbacon changed the title Change: add additional information to environment services Improvement: add additional information to environment services Feb 6, 2024
@shreddedbacon shreddedbacon force-pushed the environment-service-extension branch 3 times, most recently from 0fbaf59 to db00109 Compare February 9, 2024 05:23
@shreddedbacon shreddedbacon force-pushed the environment-service-extension branch from db00109 to b69d70d Compare February 9, 2024 06:58
@tobybellwood
Copy link
Member

tobybellwood commented Feb 11, 2024

Looking at this, i've got questions about how the name and type are (or should be) populated.

A new core (in testing) - without a remote sending any updated information, appears to be putting the type into the name column

Screenshot from 2024-02-09 18-10-22

Is it the addOrUpdateEnvironmentService mutation on environment 3 in the screenshot that's setting the name and type?

I can't fully understand how the name currently gets set - it appears to be a combination of type and service name, but maybe this is our chance to straighten it all up?
image

@shreddedbacon
Copy link
Member Author

Looking at this, i've got questions about how the name and type are (or should be) populated.

A new core (in testing) - without a remote sending any updated information, appears to be putting the type into the name column

Is it the addOrUpdateEnvironmentService mutation on environment 3 in the screenshot that's setting the name and type?

I can't fully understand how the name currently gets set - it appears to be a combination of type and service name, but maybe this is our chance to straighten it all up?

Only the name will be populated at the moment as there is no version of the remote-controller published that contains the code to set the new fields. The current controller sends this information back currently for the name.

The proposed changes to remote are still to be decided, but a rough first run is here

@shreddedbacon shreddedbacon force-pushed the environment-service-extension branch from b69d70d to 2509e97 Compare February 11, 2024 21:25
@shreddedbacon
Copy link
Member Author

So after digging, what you're seeing is just something that has always been a problem, the remote-controller uses the "container name" which is incorrectly consumed (and has been since forever) the current controller sends the same container name https://github.com/uselagoon/remote-controller/blob/v0.15.4/controllers/v1beta1/podmonitor_buildhandlers.go#L299-L314

So the opensearch ones uses the chart name, which seems to be something we haven't really managed to get right in the templates for a lot of the helm charts we currently have, mixed bag for different services
https://github.com/uselagoon/build-deploy-tool/blob/main/legacy/helmcharts/opensearch/templates/deployment.yaml#L69
https://github.com/uselagoon/build-deploy-tool/blob/main/legacy/helmcharts/opensearch/Chart.yaml#L2

Copy link
Member

@tobybellwood tobybellwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - I'm happy with this. Acknowledging that until we get full service/container support in the build tool and remote it won't be fully active.

Migration named to fit sequentially

@tobybellwood tobybellwood merged commit f443a51 into main Feb 12, 2024
1 check passed
@tobybellwood tobybellwood deleted the environment-service-extension branch February 12, 2024 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants