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

[RFC][Roadmap] TVM Continuous Integration & Testing Roadmap #54

Merged
merged 4 commits into from
Feb 9, 2022

Conversation

denise-k
Copy link
Contributor

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

LGTM, @leandron can you take a look here too?

@denise-k
Copy link
Contributor Author

denise-k commented Feb 1, 2022

@areusch can you get some additional reviews on this? thanks!

@areusch
Copy link
Contributor

areusch commented Feb 3, 2022

@Mousius since @leandron is a bit busy would you like to review?

@areusch areusch requested a review from Mousius February 3, 2022 22:02
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

Sorry it took me a long time to review the RFC.

In general it looks good to me all the proposed ideas. The only thing I think we should add, is a "Security" item, pointing out that we should make sure our infra runs on actively supported versions of tools (meaning they receive security patches) and deprecation mechanisms are in place so that we don't keep old versions around after they reached end-of-life (EOL) status.

Examples of that are currently our usage of Python 3.6 and Ubuntu 16.04.

Apart from that, I'm happy for this RFC to be merged, pending your analysis of my suggestion above.

@Mousius
Copy link
Member

Mousius commented Feb 4, 2022

As an extension of @leandron 's comment above, we should apply the same to packages we install and use as part of TVM - at the bare minimum we should have tools to check packages haven't notified of major vulnerabilities and basic static analysis tools to catch easy to spot vulnerable code. As part of this we need some kind of mechanism for locking versions and performing updates on them regularly (I remember @areusch was keen on poetry at one point 😉).

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

LGTM, I agree with the high level bullets about Security. Thanks for organising this @denise-k!

@areusch
Copy link
Contributor

areusch commented Feb 4, 2022

@leandron @Mousius thanks for taking a look! @denise-k updated the RFC to address and scope security. I agree this is important. I think this covers the bit you're mentioning about CI security; I think given the themes of the roadmap, TVM security should fall more into a "release-oriented" roadmap. Currently we haven't specified a roadmap to hold any work around release infra. We could expand this one to hold it, but I'd rather merge this so we can make forward progress on adding the CI & Testing tasks we have now to the existing roadmap, and contemplate a release roadmap in a follow-on RFC. I do indeed want to continue hacking on my poetry-based Python dependency management thing soon.

@areusch
Copy link
Contributor

areusch commented Feb 4, 2022

since we're out of cycles to upstream tasks today, @Mousius can you take another look and make sure this agrees with you, and feel free to approve/merge if so.

@Mousius
Copy link
Member

Mousius commented Feb 7, 2022

@leandron @Mousius thanks for taking a look! @denise-k updated the RFC to address and scope security. I agree this is important. I think this covers the bit you're mentioning about CI security; I think given the themes of the roadmap, TVM security should fall more into a "release-oriented" roadmap. Currently we haven't specified a roadmap to hold any work around release infra. We could expand this one to hold it, but I'd rather merge this so we can make forward progress on adding the CI & Testing tasks we have now to the existing roadmap, and contemplate a release roadmap in a follow-on RFC. I do indeed want to continue hacking on my poetry-based Python dependency management thing soon.

Could you clarify how security is limited to a release? The tooling we use to automate detection of insecure packages and vulnerable code should be ran across all changes rather than checking it as part of a release. We should aim to keep our own CI and development environments secure as a general practice with CI automation to aid us.

@areusch
Copy link
Contributor

areusch commented Feb 7, 2022

@Mousius One way to group the various security related tasks is like so:

  • Infra work: adding e.g. vuln scanners to CI or elsewhere
  • Security problems with CI infra: issues outside the TVM codebase, but which are encountered only because we need to run a CI. Things related to e.g. Jenkins version, patching OS base box vulnerabilities, etc. Likely handled by those TVM committers working on CI.
  • Security problems with TVM dependencies: issues whose root cause are outside the TVM codebase, but which any TVM user might encounter in the course of using TVM. Ultimately resolved by updating dependencies, but likely requires some collaboration between folks working on TVM core and folks working on making a TVM release (e.g. which would actually specify the dependency versioning requirements and presumably where a future test might run)
  • Security vulnerabilities in the TVM code itself: memory overflows and kin, privilege escalation, etc. Resolved just like any other TVM change, but requires a direct resolution in the TVM codebase.

The first two problems, we can tackle without considering how to message those in a release, because they have immediate resolution. The latter two require us to consider how our release process would message these issues, whether we would cherry-pick a workaround/fix to earlier releases, etc. So let's table those two here until we sort out the release process and determine where they'll land then. It's entirely possible we should just create a separate Roadmap for security to track everything centrally, but we're just trying to land this one piece here. We added the security item to address @leandron 's comment, which I think is entirely reasonable, but I think his request was scoped to just the CI infra if I understand it correctly. Would prefer to proceed on this one and consider security holistically in a follow-on RFC.

@Mousius
Copy link
Member

Mousius commented Feb 7, 2022

@areusch helped me understand the contention here, there's a release process for managing things such as bugs, security vulnerabilities and other forms of fixes in a release cycle and accounting for all of those things in this roadmap is definitely out of scope 😸

My confusion was that "Security problems with TVM dependencies" and "Security vulnerabilities in the TVM code itself" can be monitored by automation, which is what I'm suggesting to include as part of this roadmap. The actual tactics of remediation for issues would be defined elsewhere, likely alongside release processes or as part of a another roadmap.

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

Thanks @denise-k / @areusch, apologies that this took a bit longer due to my misunderstanding!

@Mousius Mousius merged commit 41e5ba0 into apache:main Feb 9, 2022
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