Skip to content

Conversation

@MahatiC
Copy link
Member

@MahatiC MahatiC commented Jan 15, 2026

  • This PR has two changes:
    • Implements CIM volume unmount as part of container shut down process
    • Before this change, Scratch disk gets unmounted twice and it errors out on second attempt and as a result it never reaches to CIM unmount operation. This PR fixes it by removing it from ResourceCloser list as it is already unmounted previously i.e. lc.scratchMount.Release(ctx);

@MahatiC MahatiC requested a review from a team as a code owner January 15, 2026 11:50
@KenGordon
Copy link
Collaborator

I have tested this on nord1 and it works at least as well as before with the simple attestation report web server nanoserver based container.

BlockPath: physicalDevPath,
CimName: blockCimDevice.CimName,

// skip the merged cim and verify individual layer hashes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a written explanation anywhere of why we don't need to check the merged CIM root hash? I think it needs to be explained here.

log.G(ctx).Tracef("WCOWBlockCIMMounts Add { %v}", wcowBlockCimMounts)

// The block device takes some time to show up. Wait for a few seconds.
time.Sleep(2 * time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed (but not for today) this can't be a sleep before check but needs to be more like the LCOW scheme where it only sleeps if it fails. See

// todo (maksiman): add better retry logic, similar to how SCSI device mounts are
which is a bit nasty but illustrates the point.

return fmt.Errorf("failed to get CIM verification info: %w", err)
}
layerDigests[i] = cimRootDigestBytes
layerHashes[i] = base64.URLEncoding.EncodeToString(cimRootDigestBytes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed (also not for today), the layer hash strings ought to be hex encoded as per the linux ones.

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