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

Remote labels #696

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Kern--
Copy link
Contributor

@Kern-- Kern-- commented Jul 18, 2022

Previously, the demux-snapshotter had to make an http request to the
http-address-resolver to find the vsock address and metrics
configuration for a remote snapshotter. With this change, the
http-address-resolver is removed and this information is sent to the
demux-snapshotter via snapshot labels directly from the client.

The labels are sent to the demux-snapshotter in the first pepare call
at which point the demux-snapshotter will establish connection with the
remote snapshotter. The connection is then cached via the namespace key
as it was in the previous implementation for subsequent calls.

Since each microVM houses its own remote snapshotter in a clean slate on
boot, any snapshot API call before a prepare will be operating on an empty
snapshotter and therefore will be a noop/not found. Therefore, if we
don't have a connection for an API, we can treat it as the noop/not found
case.


I would like feedback on the idea here. There's a lot to cleanup including:

  1. Tests for the new pieces
  2. Setting up the eviction policy without a hardcoded port
  3. Removal of dead code (e.g. address resolver config)
  4. General cleanup/comments

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Previously, the demux-snapshotter had to make an http request to the
http-address-resolver to find the vsock address and metrics
configuration for a remote snapshotter. With this change, the
http-address-resolver is removed and this information is sent to the
demux-snapshotter via snapshot labels directly from the client.

The labels are sent to the demux-snapshotter in the first pepare call
at which point the demux-snapshotter will establish connection with the
remote snapshotter. The connection is then cached via the namespace key
as it was in the previous implementation for subsequent calls.

Since each microVM houses its own remote snapshotter in a clean slate on
boot, any snapshot API call before a prepare will be operating on an empty
snapshotter and therefore will be a noop/not found. Therefore, if we
don't have a connection for an API, we can treat it as the noop/not found
case.

Signed-off-by: Kern Walster <[email protected]>
Copy link
Contributor

@kzys kzys left a comment

Choose a reason for hiding this comment

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

The direction looks good to me.

@@ -79,6 +76,8 @@ logs:

clean:
- rm -f demux-snapshotter
# Leaving legacy binary cleanup so that existing copies can get to a clean
# state after pulling from main
- rm -f http-address-resolver
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine not careing existing copies. If we keep this line now, when would we remove the line?

@@ -97,6 +86,22 @@ func TestGuestVolumeFrom_Isolated(t *testing.T) {
require.NoErrorf(t, err, "Failed to create microVM[%s]", vmID)
defer fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})

// Add a stargz image.
// The volume directories must be specified since the host's containerd doesn't know about the image.
remoteImage := volume.FromGuestImage(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kzys This is one slight change. GuestVolumeImageProvider handles pulling the remote image in order to extract the volume. Since the pull itself is hidden inside VolumeSet.AddFrom, we need the VM to be created before creating image provider so we can give it the vsock path to be used later when the pull happens.

This means that the flow is now:

  1. Create volume set
  2. Add non-lazy volumes
  3. Prepare drive mount
  4. Launch VM
  5. Add lazy volumes
  6. Prepare lazy volumes in VM

Before we would have added lazy volumes right after adding non-lazy volumes. I think this still mostly makes sense, but I wanted to highlight that.

@kzys
Copy link
Contributor

kzys commented Jul 21, 2022

The role of the first Prepare() request is a bit concerning. We are assuming some order of calls which is less explicit than what we have currently.

If a caller violates the assumption, how does that work? Just return NotFound? Would it return something more specific? I still think the benefits of having less code/components is more worth than the risk of having cryptic errors though.

BenjaminChun pushed a commit to char-1ee/firecracker-containerd that referenced this pull request Apr 25, 2024
gVisor CRI setup needs to be done in the same place as firecracker CRI
setup, otherwise setup parts such as logger setup and orchestrator creation
are skipped.

Closes firecracker-microvm#696

Signed-off-by: Georgiy Lebedev <[email protected]>
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