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

[TEST] Broker / EdgeHub Packaging and Build Infra #3132

Closed

Conversation

and-rewsmith
Copy link
Contributor

@and-rewsmith and-rewsmith commented Jun 23, 2020

The MQTT Broker is designed to run in the same container as EdgeHub Core. Currently, we do not have the packaging logic or build infrastructure to do this.

This PR contains the following:

  • Packaging logic for edge hub and the mqtt broker to run inside the same container.We made our own watchdog for these two processes (edgehub and broker) implemented in rust.
  • New build logic for the mqtt broker which will cross compile and lock down necessary dependencies (rust toolchain, openssl, cmake) on a per-platform basis. This is modeled after the edgelet build process defined in package.sh.
  • Pipeline integration that will build and publish combined edgehub/broker images for each target platform.

Packaging logic
I created a watchdog rust program that will launch two child processes for edgehub and broker. This is the big picture:

  • Both processes run together in the same container
  • If either of these processes stop, the other is shut down gracefully and the container exits
  • If the docker container is stopped, both processes shut down gracefully
  • Handle processes appropriately (i.e. prevention of zombie processes)
  • Redirect output of both processes to stdout

Since the watchdog is a component running inside edgehub, I have refactored the directory structure of edgehub to contain subfolders: (docker, core, watchdog). This change bloated the size of the PR as I had to change many .cs and .csproj files.

New Build Logic
We needed to come up with a new build process for the broker and the watchdog. The current approach did not provide flexibility in selecting rust toolchain, openssl, cmake versions.

We modeled this new build logic after edgelet. See edgelet's package.sh for context on how edgelet cross compiles to lock down dependencies. This can mostly be replicated, but since there was no existing logic for alpine, we had to come up with something new.

Regarding alpine, we don't need to build directly on alpine. Building on bionic will also work as long as it produces a musl binary. In order to build musl artifacts on bionic, I ripped out the relevant parts of this dockerfile (line 38-70, 110-126, 145-155), which then get copied into the alpine image.

All the other build logic has precedent in edgelet.

Pipeline Integration
Since we decided not to build windows images for the broker, I have removed windows from the build and manifest steps. I then used the new build logic to build the watchdog/broker artifacts, copied them into the build context (edgehub's dotnet publish folder), then set the watchdog as the image entry point.

This change will significantly increase the build time of our build images pipeline (60+ mins). We can slightly mitigate this by reducing the 6 rust build steps to 3 (consolidate broker and watchdog cross compilations), but the cross compilation of the broker is the most costly. The best way to mitigate this is to have a base image that gets regularly uploaded to our build container registry, but then we need to deal with maintaining compatibility for all feature branches. We can handle this outside this PR.

Thoughts and lingering things

  • Our current edge hub image build approach is to copy all the contents of the edge hub publish folder into the image. The broker and watchdog artifacts are now inside this publish folder (to reduce build context size). There is currently precedent for this behavior (i.e. the dockerfile folder), however I think this is a dangerous habit because it can lead to undesirable files bloating the container size. This would be acceptable imo if we could provide some sort of exclude list, however that is not supported. Relevant issue. I tried using a rm -rf command to remove the watchdog, mqtt, and docker folders but since the build agent is amd64, it can't run this command on the arm base images. We need to either find a way around this issue or concede to a larger build context containing all the published dotnet apps.
  • We still have windows docker files checked into the repo. I think it is best to remove them, but was hesitant due to the already large size of the PR.
  • Need to handle build images release. We will end up replicating the logic in our build pipeline, but how are we going to handle the code sign? I think this might be fine since there is no signing of executables on linux but need to double check.
  • I am still doing manual testing on the broker, but there is likely some configuration that I could be missing. Let me know if you see something wrong.

Comment on lines 4 to 5
ADD ./watchdog/arm32/watchdog /usr/local/bin/watchdog
ADD ./mqtt/arm32/mqttd /usr/local/bin/mqttd
Copy link
Contributor Author

@and-rewsmith and-rewsmith Jun 23, 2020

Choose a reason for hiding this comment

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

I think we should strip these executables here and in other dockerfiles where appropriate. Unfortunately I doubt it will work here for arm because the build agent is amd64. I think we will need to do it in the build container. It looks like we can use cargo strip to simplify this:
rust-lang/cargo#3483

Comment on lines 44 to 45
CMD echo "$(date --utc +"%Y-%m-%d %H:%M:%S %:z") Starting Edge Hub" && \
/usr/local/bin/watchdog
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The broker needs to be configured to dump its state where it has permission to do so.

Comment on lines 7 to 8
all:
$(CARGO) build $(CARGOFLAGS)
Copy link
Contributor Author

@and-rewsmith and-rewsmith Jun 23, 2020

Choose a reason for hiding this comment

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

I followed the same pattern as for edgelet, but should we remove this? It might be safer to only build release since the only place this makefile is used is in the build pipeline.

thread::spawn(move || {
for _sig in signals.forever() {
println!("{:?} Received SIGTERM for watchdog", LOG_HEADER);
sigterm_listener.store(false, Ordering::Relaxed); // TODO: figure out best Ordering
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 should pick the correct ordering here.

is_edgehub_running = is_child_process_running(&mut edgehub);
is_broker_running = is_child_process_running(&mut broker);

thread::sleep(Duration::from_secs(4)); // TODO: configure wait time for poll
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 need to determine how often the watchdog should poll the processes. Any ideas?

signal::kill(Pid::from_raw(broker.id() as i32), Signal::SIGTERM).unwrap();
}

thread::sleep(Duration::from_secs(10)); // TODO: configure wait time
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 need to determine how long we will give EdgeHub and Broker to both shut down cleanly before issuing SIGKILL. Any ideas?


fn is_child_process_running(child_process: &mut Child) -> bool
{
return !child_process.try_wait().unwrap().is_some();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to determine how to handle unwrap if fails.

let mut is_broker_running = is_child_process_running(&mut broker);
if is_edgehub_running {
println!("{:?} Sending SIGTERM to Edge Hub", LOG_HEADER);
signal::kill(Pid::from_raw(edgehub.id() as i32), Signal::SIGTERM).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to determine how to handle unwrap if fails.

is_broker_running = is_child_process_running(&mut broker);
if is_edgehub_running {
println!("Killing Edge Hub");
edgehub.kill().unwrap();
Copy link
Contributor Author

@and-rewsmith and-rewsmith Jun 23, 2020

Choose a reason for hiding this comment

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

Need to determine how to handle unwrap if fails. The process might have already shut down even if the is_edgehub_running evaluates true.

Comment on lines +117 to +120
OPENSSL_VERSION=1.1.1g
MDBOOK_VERSION=0.3.7
CARGO_ABOUT_VERSION=0.2.2
CARGO_DENY_VERSION=0.6.7
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 need to find a way to handle these important dependencies since they will get updated. Any ideas? We can elevate to the top of file for clarity, but that will separate it from the alpine logic.


WORKDIR /app

COPY $EXE_DIR/ ./
# Copy edge hub artifacts and remove unnecessary items, leaving just edge hub core dlls
COPY $EXE_DIR/ ./
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 need to remove docker files, mqtt binary, watchdog binary. I tried adding a rm step, but since the builder agent is amd64 this won't work on arm. Any ideas?

Comment on lines 14 to 20
cp edge-hub/watchdog/linux/target/x86_64-unknown-linux-musl/release/watchdog $(Build.BinariesDirectory)/$(edgehub.build.path)/watchdog/amd64
cp edge-hub/watchdog/linux/target/armv7-unknown-linux-gnueabihf/release/watchdog $(Build.BinariesDirectory)/$(edgehub.build.path)/watchdog/arm32
cp edge-hub/watchdog/linux/target/aarch64-unknown-linux-gnu/release/watchdog $(Build.BinariesDirectory)/$(edgehub.build.path)/watchdog/arm64
cp mqtt/target/x86_64-unknown-linux-musl/release/mqttd $(Build.BinariesDirectory)/$(edgehub.build.path)/mqtt/amd64
cp mqtt/target/armv7-unknown-linux-gnueabihf/release/mqttd $(Build.BinariesDirectory)/$(edgehub.build.path)/mqtt/arm32
cp mqtt/target/aarch64-unknown-linux-gnu/release/mqttd $(Build.BinariesDirectory)/$(edgehub.build.path)/mqtt/arm64
displayName: Copy watchdog and broker artifacts to dockerfile build context
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: we could clean this up further

@@ -0,0 +1,295 @@
#!/bin/bash
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should consolidate this with the script in edgelet because there is a lot of edgelet specific logic.

…ewsmith/iotedge into andsmi/broker-edgehub-packaging-pr
@and-rewsmith and-rewsmith marked this pull request as ready for review June 24, 2020 07:25
@and-rewsmith and-rewsmith marked this pull request as draft June 24, 2020 22:49
@and-rewsmith
Copy link
Contributor Author

I'm opening 3 smaller PRs so please don't review this.

@and-rewsmith and-rewsmith changed the title Broker / EdgeHub Packaging and Build Infra For Testing: Broker / EdgeHub Packaging and Build Infra Jul 7, 2020
@and-rewsmith and-rewsmith changed the title For Testing: Broker / EdgeHub Packaging and Build Infra [TEST] Broker / EdgeHub Packaging and Build Infra Jul 7, 2020
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.

1 participant