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

Migrate to Dotnet 6 #6214

Merged
merged 25 commits into from
Apr 13, 2022
Merged

Migrate to Dotnet 6 #6214

merged 25 commits into from
Apr 13, 2022

Conversation

and-rewsmith
Copy link
Contributor

@and-rewsmith and-rewsmith commented Mar 16, 2022

This PR migrates main branch from dotnet 3.1 to dotnet 6.0. This is working off of Damon's prior PR here:
#6185

Code changes on top of the linked PR are minor. Only notable things:

  1. Removed symlinks from arm64 dockerfiles that are no longer needed
  2. TempfilterFunctions module is still on dotnet 3, as base image doesn't support dotnet 6 for arm32
  3. EdgeHub watchdog has to shell out to sleep rather than call thread::sleep() to bypass a bug in rust. Logged a bug with rust repo.
  4. Various retries added in E2E test code to get around problems caused by dotnet 6. Logged bugs with SDK team where appropriate.
  5. Upgrade rust artifact build OS for amd64/arm* platforms to ubuntu 20 (from ubuntu 18).

Passing pipelines:

  1. Build Images
  2. CI
  3. E2E
  4. Nested E2E
  5. Connectivity
  6. Build Images Release

Azure IoT Edge PR checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines and Best Practices

  • I have read the contribution guidelines.
  • Title of the pull request is clear and informative.
  • Description of the pull request includes a concise summary of the enhancement or bug fix.

Testing Guidelines

  • Pull request includes test coverage for the included changes.
  • Description of the pull request includes
    • concise summary of tests added/modified
    • local testing done.

Draft PRs

  • Open the PR in Draft mode if it is:
    • Work in progress or not intended to be merged.
    • Encountering multiple pipeline failures and working on fixes.

Note: We use the kodiakhq bot to merge PRs once the necessary checks and approvals are in place. When it merges a PR, kodiakhq converts the PR title to the commit title, PR description to the commit description, and squashes all the commits in the PR to a single commit. The net effect is that entire PR becomes a single commit. Please follow the best practices mentioned here for the PR title and description

@and-rewsmith and-rewsmith mentioned this pull request Mar 16, 2022
6 tasks
@and-rewsmith and-rewsmith force-pushed the andsmi/net6 branch 4 times, most recently from 01d58db to 9ea9647 Compare March 29, 2022 23:05
.output()?;

Ok(())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could only do this for debian 10. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern with this is a possible performance penalty. Did we try other versions of sleep, tokio or futures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We considered introducing tokio runtime but thought it was better to put this in temporarily for now then switch back to thread::sleep when rust bug was fixed. However we recently found that we are removing broker from 1.3 codebase, which means the whole watchdog is disappearing.

async () =>
{
await client.SetMethodHandlerAsync(nameof(DirectMethod), DirectMethod, null, token);
}, token);
Copy link
Contributor Author

@and-rewsmith and-rewsmith Apr 8, 2022

Choose a reason for hiding this comment

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

FYA, added this retry. I failed to reproduce this locally.

{
return await Task.Run(async () => await client.GetRuntimeInformationAsync(), eachRequestCancellationToken);
}, token);
}
Copy link
Contributor Author

@and-rewsmith and-rewsmith Apr 8, 2022

Choose a reason for hiding this comment

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

FYA, added this retry. I failed to reproduce this locally.

Copy link
Contributor

@nimanch nimanch Apr 12, 2022

Choose a reason for hiding this comment

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

If this did not happen again, My preference would be to remove the retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It happens in the pipeline sometimes so I think we need to keep the retry.

@and-rewsmith and-rewsmith marked this pull request as ready for review April 8, 2022 15:32
@and-rewsmith
Copy link
Contributor Author

To summarize the new issues that came with this dotnet 6 upgrade:

  1. Debian 10 cannot call thread::sleep() without panicking. This results in EdgeHub container crashing. Bug logged here on rust repo. I will followup on this to make sure the issue gets fixed, and we will be able to remove the shell-out to bash sleep eventually.
  2. Setting the direct method handler via a DeviceClient sdk will throw SslStream disposed. I have only seen this happen in the test framework (with a weird stack trace) and wasn't able to reproduce this locally. Still logged a bug on sdk team here. This is a bit concerning because EdgeHub internally uses the problematic DirectMethod set method handler. I have added a retry in the tests for setting the direct method handler.
  3. Leaf device gets a TLS auth error when it expects to get an authorization error. The TLS error says that an unexpected message came from the server (EdgeHub). I thought this was concerning, but wasn't able to reproduce it outside of a test agent (even running the E2E tests on my dev pi). Didn't log a bug on SDK team because of EdgeHub involvement. Weirdly, this also happens in the set direct method handler. I have added a retry in the tests for setting the direct method handler.
  4. Getting the event hub partitions will fail due to timeout. I added custom retry logic.

@and-rewsmith and-rewsmith requested a review from damonbarry April 8, 2022 21:13
Copy link
Member

@damonbarry damonbarry left a comment

Choose a reason for hiding this comment

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

Looks good! Just one comment...

@and-rewsmith and-rewsmith requested a review from damonbarry April 11, 2022 22:11
damonbarry
damonbarry previously approved these changes Apr 11, 2022
@@ -11,7 +11,7 @@ jobs:
################################################################################
displayName: Check pipeline preconditions (changes ARE NOT in either edgelet, docs, or mqtt folder)
pool:
vmImage: "ubuntu-18.04"
vmImage: "ubuntu-20.04"
Copy link
Contributor

Choose a reason for hiding this comment

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

does dotnet6 only run on ubuntu 20.04 or this was just an unrelated enhancement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated enhancement.

@@ -139,7 +139,7 @@ steps:
- pwsh: |
$manifestSignerClientDir = '$(Build.SourcesDirectory)/samples/dotnet/ManifestSignerClient'
dotnet build $manifestSignerClientDir
$manifestSignerClientBinDir = Convert-Path "$manifestSignerClientDir/bin/Debug/netcoreapp3.1"
$manifestSignerClientBinDir = Convert-Path "$manifestSignerClientDir/bin/Debug/net6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to update e2e-setup-base-image-update-release.yaml as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I think so, good catch. Also compare-compatability.yaml

nimanch
nimanch previously approved these changes Apr 12, 2022
damonbarry
damonbarry previously approved these changes Apr 12, 2022
@kodiakhq kodiakhq bot merged commit 37234e0 into Azure:main Apr 13, 2022
damonbarry added a commit to damonbarry/iotedge that referenced this pull request Apr 15, 2022
This PR migrates main branch from dotnet 3.1 to dotnet 6.0. This is working off of Damon's prior PR here:
Azure#6185

Code changes on top of the linked PR are minor. Only notable things:
1. Removed symlinks from arm64 dockerfiles that are no longer needed
2. TempfilterFunctions module is still on dotnet 3, as base image doesn't support dotnet 6 for arm32
3. EdgeHub watchdog has to shell out to sleep rather than call `thread::sleep()` to bypass a bug in rust. Logged a bug with rust repo.
4. Various retries added in E2E test code to get around problems caused by dotnet 6. Logged bugs with SDK team where appropriate.
5. Upgrade rust artifact build OS for amd64/arm* platforms to ubuntu 20 (from ubuntu 18).

Passing pipelines:
1. [Build Images](https://dev.azure.com/msazure/One/_build/results?buildId=54001600&view=results)
2. [CI](https://dev.azure.com/msazure/One/_build/results?buildId=53619492&view=results)
6. [E2E](https://dev.azure.com/msazure/One/_build/results?buildId=54004817&view=results)
7. [Nested E2E](https://dev.azure.com/msazure/One/_build/results?buildId=53668874&view=results)
8. [Connectivity](https://dev.azure.com/msazure/One/_build/results?buildId=53834451&view=results)
9. [Build Images Release](https://dev.azure.com/msazure/One/_build/results?buildId=53619579&view=results)
nimanch added a commit that referenced this pull request Apr 19, 2022
* Longhaul: Fix tolerance for broker enabled messaging (#6183) (#6239)

Cherry pick #6183

## Azure IoT Edge PR checklist:

* Move edge-modules/MetricsCollector to test/modules/TestMetricsCollector (#6240)

Move test metrics collector into test directory for clarity and consistency w/ 1.1 branch.

These changes were tested by running a build images pipeline followed by E2E and Connectivity Test pipelines.

## Azure IoT Edge PR checklist:

* [Tools] Addition of libssl1.0 check and refactor the script (#6242)

* Added checks for libssl 1.0 when 1.1 is not present
* Refactored shared lib check to simplify the checks
* Grouped all variables to the top of files
* Grouped all the function's util functions and renamed them
* Removed storage check logic to a separate function so only the names of the function remain in aziotedge_check() for code readability and debug purposes

* [main] Upgrade Rust toolchain (#6206)

This is our monthly toolchain upgrade to the latest stable version. Since iotedge/edgelet is a binary package, we want to be tracking Rust compiler releases closely in the event a stdlib vulnerability is found. We do not pin to "stable", however, since that has caused pipeline breakage when some code patterns are made illegal (like in the upgrade from 1.47 to 1.48 [^0]) or, more often, clippy lints are added.

- Add `#[must_use]` directives on methods returning `Self`
- Change `unwrap_or_else(...::new)` to `unwrap_or_default` where appropriate
- Box `CertIssuanceOptions` in `EdgeCa` enum due to large variant size difference otherwise

## Azure IoT Edge PR checklist:

* Allow apt-get to wait up to 5 minutes on lock. (#6259)

Allow apt-get to wait up to 5 minutes on lock. 
This option should work on apt-get Version 1.9.11 or later,
but experimentation shows this only works for "install" and not "update" so it may not fix all problems

* [main] Add must_use attributes to methods returning Self (#6268)

Satisfy clippy.

## Azure IoT Edge PR checklist:

* Document for IoT edge compatibility script (#6258)

Documentation on usage of Compatibility script.

* E2E: Fix proxy Manifest Trust Setup build step (#6266)

Manifest Trust Setup build step isn't using proxy env vars. Fixing.


## Azure IoT Edge PR checklist:

* [Platform Compatibility] Fix Resource Profiling for IoT Edge (#6271)

Enhancements
1. Always get support bundle logs for analysis
2. Use the Configured Edge Agent Image for Bootstrapping IoT Edge

## Azure IoT Edge PR checklist:

* [main] Device product information (#6262)

Currently, the edge agent encodes some runtime information (e.g. EdgeAgent and .NET versions) as part of the product information connection parameter, which can then be used to examine device characteristics from management interfaces. Some users desire the ability to add system-level properties--such as kernel version--to the product information. This PR adds a configuration option for custom product information strings as well as an automated option which augments the existing runtime information with the following system parameters:
- kernel name, release, architecture
- OS name, release
- system name, version, vendor

Generated files:
- edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Edgelet/version_2021_12_07/generatedCode/EdgeletHttpClient.cs

Largely duplicated files:
- edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Edgelet/version_2021_12_07/ModuleManagementHttpClient.cs
- edgelet/api/managementVersion_2021_12_07.yaml

### General Guidelines and Best Practices
- [x] I have read the [contribution guidelines](https://github.com/azure/iotedge#contributing).
- [x] Title of the pull request is clear and informative.
- [x] Description of the pull request includes a concise summary of the enhancement or bug fix.

### Testing Guidelines
- [x] Pull request includes test coverage for the included changes.
- Description of the pull request includes 
	- [ ] concise summary of tests added/modified
	- [x] local testing done.

* [main] Upgrade to latest Rust version (#6278)

This is our monthly toolchain upgrade to the latest stable version. Since iotedge/edgelet is a binary package, we want to be tracking Rust compiler releases closely in the event a stdlib vulnerability is found. We do not pin to "stable", however, since that has caused pipeline breakage when some code patterns are made illegal (like in the upgrade from 1.47 to 1.48 [^0]) or, more often, clippy lints are added.

- Change from `cloned()` to `copied()` since we can and to work around ICE: rust-lang/rust-clippy#8662

[^0]: https://github.com/rust-lang/rust/blob/master/RELEASES.md#compatibility-notes-11
  Namely, the point on `mem::uninitialized`.

## Azure IoT Edge PR checklist:

* Platform Compatibility : Use Docker Socket For Docker Engine API Check (#6281)

A Device may not necessarily support Docker CLI to query Docker Engine API , even though it has a valid docker socket to communicate with. This PR Adds support for querying Docker Engine API from docker socket. 

Also fixes an issue where we were incorrectly querying Client API Version instead of Server API Version.


## Azure IoT Edge PR checklist:

* UI for Platform compatibility script (#6274)

* include cross and checks
* Changing magenta color as it was over powering all the other messages
* changes of certain messaging to debug logs.

* [main] Rust toolchain upgrade fixes (#6283)

Continuation of #6278.

Resolves clippy failures not caught due to pipelines not triggering on changes to `rust-toolchain.toml`.

## Azure IoT Edge PR checklist:

* Documentation on Memory and Storage profiling of IoT edge (#6254)

* Setup of the profiling platform
* Metrics used for profiling
* Summary of the numbers after profiling
* How compatibility scripts uses these metrics for doing memory and storage checks

* Migrate to Dotnet 6 (#6214)

This PR migrates main branch from dotnet 3.1 to dotnet 6.0. This is working off of Damon's prior PR here:
#6185

Code changes on top of the linked PR are minor. Only notable things:
1. Removed symlinks from arm64 dockerfiles that are no longer needed
2. TempfilterFunctions module is still on dotnet 3, as base image doesn't support dotnet 6 for arm32
3. EdgeHub watchdog has to shell out to sleep rather than call `thread::sleep()` to bypass a bug in rust. Logged a bug with rust repo.
4. Various retries added in E2E test code to get around problems caused by dotnet 6. Logged bugs with SDK team where appropriate.
5. Upgrade rust artifact build OS for amd64/arm* platforms to ubuntu 20 (from ubuntu 18).

Passing pipelines:
1. [Build Images](https://dev.azure.com/msazure/One/_build/results?buildId=54001600&view=results)
2. [CI](https://dev.azure.com/msazure/One/_build/results?buildId=53619492&view=results)
6. [E2E](https://dev.azure.com/msazure/One/_build/results?buildId=54004817&view=results)
7. [Nested E2E](https://dev.azure.com/msazure/One/_build/results?buildId=53668874&view=results)
8. [Connectivity](https://dev.azure.com/msazure/One/_build/results?buildId=53834451&view=results)
9. [Build Images Release](https://dev.azure.com/msazure/One/_build/results?buildId=53619579&view=results)



## Azure IoT Edge PR checklist:

* Install .NET 6 in IoT Edge Compatibility Pipeline (#6286)

* Enhance Docker Check

* Use .NET 6

* [Platform Compatibility] Update aka.ms links (#6285)

Update Platform Compatibility Tool and Documentation with correct aka.ms links

## Azure IoT Edge PR checklist:

* [main] Remove failure dependency (#6282)

The `failure` crate is deprecated and has been superseded by `thiserror` and `anyhow`. These changes remove dependencies on `failure` and replace them by `thiserror` and `anyhow` as appropriate.

- Module{,Registry,Runtime}, MakeModuleRuntime no longer have an `Error` type parameter
- Remove unused submodules of edgelet-core

## Azure IoT Edge PR checklist:

* api proxy image update (#6265)

Content:
Updating the API proxy image because of new alpine version.

Tests:
Tested on amd64 in nested pipeline.

Co-authored-by: Andrew Smith <als5ev@virginia.edu>
Co-authored-by: Noel Campbell <7676015+nlcamp@users.noreply.github.com>
Co-authored-by: gaya <33068020+ggjjj@users.noreply.github.com>
Co-authored-by: onalante-msft <89409054+onalante-msft@users.noreply.github.com>
Co-authored-by: David Robson <darobs@microsoft.com>
Co-authored-by: hugues bouvier <33438817+huguesBouvier@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants