PelionIoT / distro-pelion-edge

Scripts for generating pelion-edge deb (Ubuntu) installer
Apache License 2.0
1 stars 2 forks source link

Add pe-terminal package to enable go-based relay-term #191

Closed adwardstark closed 2 years ago

adwardstark commented 3 years ago

This PR adds go-based relay-term package called pe-terminal which will be enabled by default, disabling the old nodejs one instead of removing it, so that we can bring up the old one incase some issue is encountered with the new one.

adwardstark commented 3 years ago

See a similar PR for snap-pelion-edge: https://github.com/PelionIoT/snap-pelion-edge/pull/391

adwardstark commented 3 years ago

@costanic I've added the source-tag and made the suggested changes, this PR is ready for review. Although the builds are failing due to some check, is there something I might be missing? Fixed the build.

adwardstark commented 3 years ago

@costanic Once this package is added we'll have two terminal-clients active, one is the old nodejs-relay-term and other is this one which can cause conflicts in connection, as part of adding this we also need to disable the old one [not removing for now]. So can you suggest me something there? In snap I was able to disable it using install-hook but in distro i'm not sure how can I do that?

costanic commented 3 years ago

two terminal-clients active

@adwardstark A couple of options come to mind, there's not really a single correct way to do this since it may not be an issue for users depending on how they install pelion-edge in general.

  1. First, you could simply document the problem in the README because it won't be an issue for normal users who install pelion-edge through the metapackage. In other words, dual terminal-clients can't happen unless a user takes specific action to manually install the pe-terminal package. You could warn developers in the install section of the README since it recommends to install *.deb. You can either direct developers to specifically ignore the pe-terminal package when installing the packages or to manually disable it after installation.

  2. If you wanted to do something in code, you could do something like in this post. See also the --no-enableand --no-start options. Here is an example. Keep in mind you'll need to roll this back when you make pe-terminal the default.

When you're ready for pe-terminal to be the default client for all users, then you'll need to swap out global-node-modules for pe-terminal in the metapackage and delete the global-node-modules package files, assuming global-node-modules has nothing else of value. If you still want to keep parts of global-node-modules, then remove only the relay-term files from the install script.

adwardstark commented 3 years ago

two terminal-clients active

@adwardstark A couple of options come to mind, there's not really a single correct way to do this since it may not be an issue for users depending on how they install pelion-edge in general.

  1. First, you could simply document the problem in the README because it won't be an issue for normal users who install pelion-edge through the metapackage. In other words, dual terminal-clients can't happen unless a user takes specific action to manually install the pe-terminal package. You could warn developers in the install section of the README since it recommends to install *.deb. You can either direct developers to specifically ignore the pe-terminal package when installing the packages or to manually disable it after installation.
  2. If you wanted to do something in code, you could do something like in this post. See also the --no-enableand --no-start options. Here is an example. Keep in mind you'll need to roll this back when you make pe-terminal the default.

When you're ready for pe-terminal to be the default client for all users, then you'll need to swap out global-node-modules for pe-terminal in the metapackage and delete the global-node-modules package files, assuming global-node-modules has nothing else of value. If you still want to keep parts of global-node-modules, then remove only the relay-term files from the install script.

@costanic I think there was a misunderstanding, we want to make pe-terminal as default and keep relay-term as a backup option incase of any issues encountered with pe-terminal, so that users can hop-back to relay-term anytime.

Looking at global-node-modules rules, I can see we're configuring relay-term service there, so is it possible if we use --no-enable and --no-start options there? Will it disable relay-term?

costanic commented 3 years ago

@costanic I think there was a misunderstanding, we want to make pe-terminal as default and keep relay-term as a backup option incase of any issues encountered with pe-terminal, so that users can hop-back to relay-term anytime.

Looking at global-node-modules rules, I can see we're configuring relay-term service there, so is it possible if we use --no-enable and --no-start options there? Will it disable relay-term?

@adwardstark Sure, you can use the same technique on the relay-term service instead of pe-terminal, but I don't agree with this approach. Is pe-terminal not ready for production? If it is, then we don't need to maintain relay-term. If not, then it's probably not ready to integrate here. Even so, have you thought about how the user would enable relay-term as a backup? I don't see anything here that talks about that use-case. Are you assuming physical access?

adwardstark commented 3 years ago

@costanic pe-terminal is production ready but we wanted to do a test drive in the dev-builds and use it internally before enabling it in production builds of pelion-edge. So here the "user" audience is developers. @ygoyal18 this was the intention right?

costanic commented 3 years ago

@costanic pe-terminal is production ready but we wanted to do a test drive in the dev-builds and use it internally before enabling it in production builds of pelion-edge. So here the "user" audience is developers. @ygoyal18 this was the intention right?

@adwardstark in that case I suggest:

  1. Create a migration guide for existing customers. Unless this change is completely transparent to customers, they will need a deprecation notice for relay-term and will need to know how to migrate their config from relay-term to pe-terminal in addition to any expectations or potential issues around use-cases in relay-term vs pe-terminal. The update path will need careful testing because we have customers that are using relay-term. What happens when they apt-get upgrade? We have to be a lot more careful here than with the snap build because the snap is conceptually a single package and we have a lot more control over the update path.

  2. Merge the pe-terminal package to dev branch but leave it disabled. This includes leaving it out of the metapackage, leaving relay-term enabled by default, and making sure that pe-terminal process isn't started by default even when manually installed and requires the user to take further action to systemctl enable and systemctl start pe-terminal after installation. This provides protection on the dev branch from having two clients enabled if users install *.deb and prevents us from creating a condition where dev branch is not in a release-ready state.

  3. Create a topic branch with pe-terminal enabled by default, including adding it to the metapackage. Relay-term must be removed from the metapackage and should be disabled by default. This is the branch to share with your testers.

whygoyal commented 3 years ago

@costanic After doing the risk assessment, we have decided to go with pe-terminal on LmP Yocto PE 2.5 release (ETA Oct 20th) and remove relay-term. As in terms of functionality, it's a 1:1 replacement and the same has been validated by QA team, it is safe to swap them out. From the user standpoint, there won't be any difference. And if there is regression, then we will handle it on per case basis.

We are taking this bold step for LmP Yocto package because we want to get rid of node.js completely from the image and reduce the user burden of having a powerful build machine to compile Yocto - https://developer.pelion.com/docs/device-management-edge/2.4/quick-start/lmp-quick-start.html#build-machine-requirements. This has been a concern for some customers and project owners have decided to fix it asap.

We can take different approaches for snap and distro as the initial conditions are different. What do you suggest should be our next action item for this PR and for snap one as well?

costanic commented 2 years ago

@whygoyal @adwardstark Circling back to this PR, since relay-term has since been replaced by pe-terminal in meta-pelion-edge, my recommendation is to do the same here. That means this PR still needs 1. remove global-node-modules and possibly pe-nodejs and 2. replace global-node-modules with pe-terminal in the metapackages.