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

feat(charm): add vm charm #7

Merged
merged 11 commits into from
Oct 4, 2023
Merged

Conversation

matthew-hagemann
Copy link
Collaborator

@matthew-hagemann matthew-hagemann commented Sep 26, 2023

Closes #4

Due to the infrastructure provisioned for the team, we were not able to move ahead with our k8s charm, and needed a charm that was set up to run on a vm instead.

@matthew-hagemann matthew-hagemann marked this pull request as draft September 26, 2023 10:12
@matthew-hagemann matthew-hagemann marked this pull request as ready for review September 28, 2023 10:10
@matthew-hagemann
Copy link
Collaborator Author

@tim-hm I believe the integration test is failing because I haven't updated the k8s charm with all the changes that have happened to the services

Copy link
Contributor

@tim-hm tim-hm left a comment

Choose a reason for hiding this comment

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

Given that we're now using a VM charm is it better to remove the k8s charm altogether?

@matthew-hagemann
Copy link
Collaborator Author

matthew-hagemann commented Sep 28, 2023

Given that we're now using a VM charm is it better to remove the k8s charm altogether?

Absolutely, especially as it falls behind what the service does. I'll make that change next 👍

@tim-hm
Copy link
Contributor

tim-hm commented Sep 28, 2023

What about the k8s charm test and the ROCKS CI action?

@matthew-hagemann
Copy link
Collaborator Author

What about the k8s charm test and the ROCKS CI action?

I found a good resource on how to do the tests, and they do seem fairly standard to include, so I'll:

  • Remove the k8s charm, ROCK and their ci actions
  • Add tests for the vm charm
  • update the ci to use them

vm_operator/README.md Outdated Show resolved Hide resolved
vm_operator/charmcraft.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,32 @@
options:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't include this much config - it might sound odd but you want to be exposing as little config as possible to keep the operator code simple.

I'd consider dropping:

  • app-host
  • app-jwt-secret - this is probably better autogenerated?
  • app-name
  • app-port - unless you're thinking of co-locating this unit on machines with other services on 443?

Both the postgres options need to be removed - this should be handled by relations to postgres.

With squid, it's worth asking IS, as I believe you should also be able to get this from a relation, though I could be wrong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Following up - some of these don't actually seem to be used at all in the code.

I'd recommend the following:

  • env - keep this, with some code to validate the entry as either prod or dev
  • log_level - again with some validation of the valid options

Comment on lines 60 to 77
self.unit.status = MaintenanceStatus("Installing rustc, cargo and other dependencies")

# Install via apt
self._install_apt_packages(["curl", "git", "gcc", "libssl-dev", "pkg-config","protobuf-compiler"])

# Ensure squid proxy, done after apt to not interfere
self._set_squid_proxy()

# Curl minial rust toolchain
try:
check_output("curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal", shell=True)
self._stored.install_completed = True
self.unit.status = MaintenanceStatus("Installation complete, waiting for database.")

except CalledProcessError as e:
logger.error(f"Curl command failed with error code {e.returncode}")
self.unit.status = BlockedStatus("Curl command failed")
return
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty heavy, and you'll end up with the whole toolchain installed on the machines running in production.

If you're planning on running on VMs in the long term, I'd recommend building this as a snap, then installing the snap package (pinned to a specific revision) in your charm. FWIW you can use the same parts definition from the removed rockcraft.yaml to build the snap :)

self.unit.status = BlockedStatus("Curl command failed")
return

def _render_systemd_unit(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you build with a snap, this can be taken care of for you automatically by snapd

@jnsgruk
Copy link
Contributor

jnsgruk commented Oct 3, 2023

@matthew-hagemann good effort on this! Charming can be a little dense to get started with! I've left some comments above and I'm happy to have a call or another round of review if you or @tim-hm think it'd be useful.

A nice clean example of using a snap within a charm and how to model it can be seen here: https://github.com/jnsgruk/parca-operator

@matthew-hagemann matthew-hagemann merged commit 9f65541 into ubuntu:dev Oct 4, 2023
3 checks passed
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.

We need a second charm that can run on a vm
3 participants