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

vm-builder: add flag to enable informant/monitor at VM startup and remove cgexec #477

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

fprasx
Copy link
Contributor

@fprasx fprasx commented Aug 16, 2023

This change enables more flexibility rolling out the monitor. We can keep using the monitor during e2e tests, while not enabling it for release builds.

This PR also removes cgexec. This functionality will be delegated to compute_ctl.

Copy link
Member

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

Seems ok, should also mention cgexec in the PR title / body / final commit message.

Makefile Outdated
@@ -196,7 +196,7 @@ docker-build-scheduler: ## Build docker image for (autoscaling) scheduler

.PHONY: docker-build-examples
docker-build-examples: vm-informant bin/vm-builder ## Build docker images for testing VMs
./bin/vm-builder -src postgres:15-bullseye -dst $(E2E_TESTS_VM_IMG)
./bin/vm-builder -src postgres:15-bullseye -dst $(E2E_TESTS_VM_IMG) -enable-informant
Copy link
Member

Choose a reason for hiding this comment

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

Need to also -enable-monitor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, just realized this. Am testing locally as it should be a little faster.

@fprasx fprasx changed the title vm-builder: add flag to enable informant/monitor at VM startup vm-builder: add flag to enable informant/monitor at VM startup and remove cgexec Aug 16, 2023
@fprasx fprasx merged commit edd72a4 into main Aug 16, 2023
@fprasx fprasx deleted the fprasx/flag-for-monitor-informant branch August 16, 2023 17:00
fprasx added a commit to neondatabase/neon that referenced this pull request Aug 18, 2023
A very slight change that allows us to configure the UID of the
neon-postgres cgroup owner. We start postgres in this cgroup so we can
scale it with the cgroups v2 api. Currently, the control plane
overwrites the entrypoint set by `vm-builder`, so `compute_ctl` (and
thus postgres), is not started in the neon-postgres cgroup. Having
`compute_ctl` start postgres in the cgroup should fix this. However, at
the moment appears like it does not have the correct permissions.
Configuring the neon-postgres UID to `postgres` (which is the UID
`compute_ctl` runs under) should hopefully fix this.


See #4920 - the PR to modify `compute_ctl` to start postgres in the
cgorup.
See: neondatabase/autoscaling#480, neondatabase/autoscaling#477. Both
these PR's are part of an effort to increase `vm-builder`'s
configurability and allow us to adjust it as we integrate in the
monitor.
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