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

gcs: Support routing container stdio to sidecar #1728

Merged
merged 1 commit into from
May 9, 2023

Conversation

ashishsachdeva
Copy link

The PR aims to add support for redirecting container stdout and stderr statements to a another container(logging container) via named pipes.
This is controlled by set of annotations being passed in the spec.
When the annotation is set, the named pipe is created in the UVM. The workload container will start to redirect stdout and stderr statements to this named pipe and logging container will use the pipe file descriptor passed to it to read data coming through pipe.

@ashishsachdeva ashishsachdeva requested a review from a team as a code owner April 12, 2023 18:08
@kevpar
Copy link
Member

kevpar commented Apr 13, 2023

Hey Ashish,

I think this is a reasonable way to approach this for a prototype, but does not achieve the level of integration with the existing codebase that we are looking for to merge.

I don't have time to dig into the design here personally right now, but I think the following things would be good to look into:

  • We have existing infrastructure for relaying/handling IO from containers. How can we make this new "pipe to sidecar" approach fit in naturally into that? Can we generalize the current code into different implementations of a "IO handler" interface, so the new approach can just be plugged in as a new implementation?
  • I think we should consider using simple pipes (such as those returned by pipe syscall or os.Pipe) rather than a FIFO. This would likely mean we need to track the other end of the pipe through some structure, but avoids the need to interact iwht FIFOs and the filesystem.
  • We should support two separate pipes going to the sidecar container, for stdout and stderr, so it can differentiate the output streams.
  • I don't think using annotations alone to control this behavior is a good fit. Our internal CRI has support for some fields that control logging, and we should look at enhancing those to express using a sidecar container for logs. This will require some thought into how that information is passed down through CRI -> shim -> GCS. Potentially we would still use annotations at some point in here, but it's generally preferable to avoid that, since they are just untyped strings.
  • The shim code today likely assumes it has stdio pipes from the GCS. We will need to understand what changes are needed there to make sure it handles this case properly.

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

Hi Ashish,

Please review the comments I left on the PR. We will get another reviewer to take a look, and then merge this once everything looks good. I want to note we are merging this to enable initial testing with this feature, and that we will need to revisit the design of the feature in the future.

A few other things to address:

  • Please rebase into a single commit, with a good title and descriptive commit message.
  • Sign your commit with git commit -s.
  • Sign the CLA (see the bot's comment) when you get a chance.

internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/runc.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
internal/guest/runtime/runc/container.go Outdated Show resolved Hide resolved
@ashishsachdeva ashishsachdeva force-pushed the asachdev/logsredirection branch 2 times, most recently from aaca850 to 3fab6e3 Compare May 3, 2023 23:31
@ashishsachdeva ashishsachdeva changed the title Redirect stdout and stderr to another container Bare Metal Container containers stdout and stderr propagation May 3, 2023
@ashishsachdeva ashishsachdeva changed the title Bare Metal Container containers stdout and stderr propagation Bare Metal Containers stdout and stderr propagation May 3, 2023
@ashishsachdeva
Copy link
Author

@microsoft-github-policy-service agree company="Microsoft"

@microsoft-github-policy-service agree company="Microsoft"

@ashishsachdeva ashishsachdeva changed the title Bare Metal Containers stdout and stderr propagation gcs: Support routing container stdio to sidecar May 6, 2023
Signed-off-by: Ashish Sachdeva <asachdev@microsoft.com>
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

lgtm

@kevpar kevpar merged commit a1d874a into microsoft:main May 9, 2023
14 of 16 checks passed
anmaxvl pushed a commit that referenced this pull request Oct 20, 2023
This PR updates our ADO fork to commits in hcsshim up to commit hash [7769a64](7769a64). This includes support for partitioned scsi devices and ensuring filesystem format for lcow scsi devices.

Related work items: #1728, #1740, #1741, #1742, #1743, #1744, #1745, #1747, #1748, #1749, #1750, #1752, #1754, #1756, #1757, #1767, #1769, #1771, #1772, #1773, #1779
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.

None yet

4 participants