Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

rootfs: Support agent tracing #200

Closed
wants to merge 3 commits into from
Closed

rootfs: Support agent tracing #200

wants to merge 3 commits into from

Conversation

jodh-intel
Copy link
Contributor

Add AGENT_TRACE=yes option to build a rootfs with agent tracing support.

Fixes #199.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

@jodh-intel
Copy link
Contributor Author

Blocked on kata-containers/agent#415.

@jodh-intel jodh-intel mentioned this pull request Nov 19, 2018
Merged
grahamwhaley
grahamwhaley approved these changes Nov 19, 2018
View reviewed changes
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

changes look good to me.
nice cleanup pickups as well.
lgtm

rootfs-builder/rootfs.sh Outdated
@@ -71,6 +72,11 @@ AGENT_INIT When set to "yes", use ${AGENT_BIN} as init process in place
of systemd.
Default value: no

AGENT_TRACE When set to "yes", create a rootfs containing additional
elements to support tracing the agent using https://jaegertracing.io.
Incompatible with AGENT_INIT="yes".
Copy link
Contributor

Choose a reason for hiding this comment

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

single whitespace indent anomoly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@grahamwhaley
Copy link
Contributor

but you tripped over your own travis URL check :-)

INFO: Checking all document URLs
ERROR: Invalid URL 'https://github.com/kata-containers/agent/blob/master/TRACING.md' found in the following files:

@jodh-intel
Copy link
Contributor Author

@grahamwhaley - yep, that was expected - see blocked comment above ;)

@jodh-intel
Copy link
Contributor Author

Added dnm to make it clearer there is a block on the agent PR.

@jodh-intel
Copy link
Contributor Author

This PR is still blocked on kata-containers/agent#415.

This was referenced Dec 6, 2018
Closed
Closed
@jodh-intel
Copy link
Contributor Author

@sboeuf, @egernst - ptal.

ghost
ghost reviewed Dec 18, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

a minor comment lgtm otherwise

rootfs-builder/rootfs.sh Show resolved Hide resolved
@jodh-intel
Copy link
Contributor Author

Branch updated for latest agent tracing branch (kata-containers/agent#415).

@jodh-intel jodh-intel mentioned this pull request Mar 4, 2019
Closed
@jodh-intel
Copy link
Contributor Author

Please do NOT remove the do-not-merge label as this PR should not land atm.

If it does land, it will currently break agent output since kata-redirect-agent-output-to-journal.conf will send all output to the journal, which for a default (proxy) setup is the same as redirecting the output to /dev/null.

See: kata-containers/agent#484.

@egernst
Copy link
Member

egernst commented Mar 26, 2019

@jodh-intel What's the relevance/status of this one?

@jodh-intel
Copy link
Contributor Author

@egernst - it's sitting here "semi-blocked" on kata-containers/runtime#1352; we could land this PR now, but as we're about to rework the agent changes for opencensus, there may be no point as this PR won't work with that. I'll leave it up to @mcastelino to decide though.

Related: kata-containers/runtime#1369.

@jodh-intel
Copy link
Contributor Author

kata-containers/agent#415 has now landed so this can now land.

/retest

@jodh-intel
rootfs: Add seccomp for debian+suse
d7c409b 
Add missing seccomp packages for Debian and openSuSE config files.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel
rootfs: Simplify config files
c68ec02 
Use single equals in string test for consistency, replace the visually
distracting `|| true` by a single call to `true(1)` at the end of the
script. This guarantees the sourced script succeeds even if any of the
tests it makes fail.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel
Copy link
Contributor Author

Regarding my comment here: #200 (comment), I've reworked the PR slightly so that by default the agent output will not be redirected to the journal. Now that kata-containers/shim#172 has landed, this is the behaviour we want I think.

Note also: kata-containers/agent#547.

@jodh-intel
Copy link
Contributor Author

/retest

amshinde
amshinde reviewed May 7, 2019
View reviewed changes
rootfs-builder/alpine/config.sh
@@ -24,6 +24,7 @@ INIT_PROCESS=kata-agent
ARCH_EXCLUDE_LIST=()

[ "$SECCOMP" = "yes" ] && PACKAGES+=" libseccomp"
[ "$AGENT_TRACE" = "yes" ] && PACKAGES+=" socat"
Copy link
Member

Choose a reason for hiding this comment

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

@jodh-intel Does this need to go to the Dockerfiles as well? I am not clear if the Dockerfiles are used when USE_DOCKER is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really as the docker files are mostly static - we want socat to be added conditionally to avoid bloat at this stage.

@jodh-intel
rootfs: Support agent tracing
0059ca2 
Add `AGENT_TRACE=yes` option to build a rootfs with agent tracing support.

Fixes kata-containers#199.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel
Copy link
Contributor Author

/retest

@amshinde
Copy link
Member

amshinde commented Jun 4, 2019

@jodh-intel Can you take a look at the CI failures? Will be good to land this PR.
I am going to rerun the CIs , in case they have gone stale.

@amshinde
Copy link
Member

amshinde commented Jun 4, 2019

/test

egernst
egernst approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@egernst egernst left a comment

Choose a reason for hiding this comment

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

Looks fine. AFAICT we don't have any tests in place which will exercise this, correct?

@egernst
Copy link
Member

egernst commented Jun 11, 2019

/retest

@egernst
Copy link
Member

egernst commented Jun 12, 2019

CI seems miserable with this change. @jcvenegas , @chavafg - seems there may be some issues w/ the osbuilder CI?

@jodh-intel
Copy link
Contributor Author

I should probably tal at this PR again tomorrow as it hasn't been touched in a while, so adding dnm for now...

@jodh-intel jodh-intel added the do-not-merge PR has problems or depends on another label Jun 12, 2019
@fidencio
Copy link
Member

fidencio commented May 3, 2021

Let me close this one as it's a do-not-merge which won't make to the 1.13.0 release.

@fidencio fidencio closed this May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@amshinde amshinde amshinde left review comments

@grahamwhaley grahamwhaley grahamwhaley approved these changes

@devimc devimc devimc approved these changes

@egernst egernst egernst approved these changes

Assignees
No one assigned
Labels
do-not-merge PR has problems or depends on another
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support agent tracing
6 participants
@jodh-intel @grahamwhaley @egernst @amshinde @fidencio @devimc