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

Update runc and containerd #28621

Merged
merged 9 commits into from
Oct 28, 2021
Merged

Conversation

jsoriano
Copy link
Member

What does this PR do?

Update runc and contained dependencies.

Why is it important?

  • Solve some dependabot warnings (the issues are starting containers and pulling images, something beats don't do).
  • runc 1.0 has been finally released, depend on it.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@jsoriano jsoriano added Team:Integrations Label for the Integrations team backport-v7.16.0 Automated backport with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Oct 25, 2021
@jsoriano jsoriano requested review from ChrsMark and a team October 25, 2021 13:49
@jsoriano jsoriano self-assigned this Oct 25, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 25, 2021
@jsoriano
Copy link
Member Author

/package

@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 25, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-27T20:20:47.312+0000

  • Duration: 128 min 41 sec

  • Commit: 2fe5c88

Test stats 🧪

Test Results
Failed 0
Passed 328
Skipped 31
Total 359

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

@jsoriano
Copy link
Member Author

Updating docker to try to solve issue with dockerlogbeat build:

[2021-10-25T14:07:07.814Z] + mage packageSystemTests
[2021-10-25T14:07:09.720Z] # github.com/docker/docker/pkg/archive
[2021-10-25T14:07:09.720Z] ../../../../../../pkg/mod/github.com/docker/[email protected]/pkg/archive/archive_unix.go:84:5: undefined: "github.com/opencontainers/runc/libcontainer/system".RunningInUserNS
[2021-10-25T14:07:09.720Z] Error: error compiling magefiles
script returned exit code 1

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

The change looks sane to me if CI agrees :).

@ChrsMark
Copy link
Member

It will also cover #28464 which has some CI issues.

Copy link
Collaborator

@jlind23 jlind23 left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrsMark
Copy link
Member

/test

@jsoriano
Copy link
Member Author

The issue with the generator is legit, I can reproduce it locally, I am trying to find what happens there.

@jsoriano
Copy link
Member Author

Ok, the problem is that the generator uses the replaces from the go.mod in the current working copy, but the beats code from upstream master. In CI this means that it uses the replaces from the branch in the PR, but the code from master. This makes some changes in the replaces complicated without breaking the community beats. In this PR I am updating docker and removing the replace, I will try to do the change without needing to change this replace, but not sure if possible.

Here it uses the current working copy: https://github.com/elastic/beats/blob/16aa13e342609adad69cc092d3649982da71ddcd/generator/common/beatgen/setup/setup.go#L48

But the code from the current upstream master:
https://github.com/elastic/beats/blob/16aa13e342609adad69cc092d3649982da71ddcd/generator/common/Makefile#L40
https://github.com/elastic/beats/blob/16aa13e342609adad69cc092d3649982da71ddcd/generator/common/beatgen/beatgen.go#L159-L166

@jsoriano jsoriano force-pushed the dependabot-runc-containerd branch from 16aa13e to 871a199 Compare October 26, 2021 11:12
@jsoriano jsoriano marked this pull request as draft October 26, 2021 14:04
@jsoriano jsoriano marked this pull request as ready for review October 26, 2021 17:18
IncludeFiles: []string{"rootfs", "config.json"},
}
archive, err := archive.TarWithOptions(buildDir, archiveOpts)
archive, err := tar(buildDir, "rootfs", "config.json")
Copy link
Member Author

Choose a reason for hiding this comment

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

@fearful-symmetry could you please take a look to the changes here?

I am doing this because updating containerd broke github.com/docker/docker/pkg/archive. The solution would be to upgrade docker, but upgrading docker breaks community beats (more info in other comment).

I saw that we are already using the tar command in other parts of this magefile, so I guess it is ok to use it here too 🙂 I gave it a try and it seems to work. Could you please double-check?

As a side advantage, after removing github.com/docker/docker/pkg/archive from here, runc doesn't appear anymore in go.mod.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also decide to update docker and break (or remove) community beats in master/8.0, but I would leave this discussion for later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gimme a few, lemme pull this down and poke around.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the build seems to work, I suspect I just did that because I'm not a fan of shelling out when I don't need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for checking!

I'm not a fan of shelling out when I don't need to.

+1 to this 🙂 But in this case we remove a problematic dependency, and we are already shelling out for other things. If this becomes problematic we can look for a different tar library in the future.

@jsoriano
Copy link
Member Author

There were additional changes since last review, could I have another round? 🙂

@jsoriano
Copy link
Member Author

E2E test failures are not related, they are also failing in master.

@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b dependabot-runc-containerd upstream/dependabot-runc-containerd
git merge upstream/master
git push upstream dependabot-runc-containerd

@jsoriano
Copy link
Member Author

/test

@jsoriano
Copy link
Member Author

E2E failures are unrelated.

@jsoriano jsoriano merged commit 97818c3 into elastic:master Oct 28, 2021
@jsoriano jsoriano deleted the dependabot-runc-containerd branch October 28, 2021 10:02
@jsoriano jsoriano added the backport-v8.0.0 Automated backport with mergify label Oct 28, 2021
mergify bot pushed a commit that referenced this pull request Oct 28, 2021
Solve some dependabot warnings (the issues are starting containers
and pulling images, something beats don't do).
Removes a dependency on runc in dockerlogbeat build scripts.

(cherry picked from commit 97818c3)

# Conflicts:
#	NOTICE.txt
#	go.mod
#	go.sum
mergify bot pushed a commit that referenced this pull request Oct 28, 2021
Solve some dependabot warnings (the issues are starting containers
and pulling images, something beats don't do).
Removes a dependency on runc in dockerlogbeat build scripts.

(cherry picked from commit 97818c3)

# Conflicts:
#	go.sum
jsoriano added a commit that referenced this pull request Oct 28, 2021
Solve some dependabot warnings (the issues are starting containers
and pulling images, something beats don't do).
Removes a dependency on runc in dockerlogbeat build scripts.

(cherry picked from commit 97818c3)

Co-authored-by: Jaime Soriano Pastor <[email protected]>
jsoriano added a commit that referenced this pull request Oct 28, 2021
Solve some dependabot warnings (the issues are starting containers
and pulling images, something beats don't do).
Removes a dependency on runc in dockerlogbeat build scripts.

(cherry picked from commit 97818c3)

Co-authored-by: Jaime Soriano Pastor <[email protected]>
Icedroid pushed a commit to Icedroid/beats that referenced this pull request Nov 1, 2021
Solve some dependabot warnings (the issues are starting containers
and pulling images, something beats don't do).
Removes a dependency on runc in dockerlogbeat build scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.16.0 Automated backport with mergify backport-v8.0.0 Automated backport with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants