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

Ensure instances are started after creation even with long running create processes WD-4050 #360

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented May 25, 2023

Done

  • subscribe to events websocket to get live updates from LXD for any lifecycle changes
  • use events to launch an instance that was created and is supposed to start.

Fixes WD-4050

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described in the Readme.
  2. Perform the following QA steps:
    • create or create+start some container and/or VMs and see the notifications work and the desired outcome is reached.

@webteam-app
Copy link

Demo starting at https://lxd-ui-360.demos.haus

src/context/instanceLoading.tsx Outdated Show resolved Hide resolved
src/context/instanceLoading.tsx Outdated Show resolved Hide resolved
src/pages/instances/Events.tsx Outdated Show resolved Hide resolved
};
ws.onmessage = (message: MessageEvent<Blob | string | null>) => {
if (typeof message.data !== "string") {
console.log("Invalid format on event api: ", message.data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log("Invalid format on event api: ", message.data);
// TODO: remove this and other console.log calls
console.log("Invalid format on event api: ", message.data);

Copy link
Collaborator Author

@edlerd edlerd May 30, 2023

Choose a reason for hiding this comment

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

The log is for discovery of errors in the future. I wouldn't want to remove this in the future. I see we added this todo comment to various places where we handle websockets and think it should be removed there as well. Or how would this be useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what's special about websockets to justify these calls to console.log, which are discouraged for production code and a bad practice in general. They leak information about the app to the user.
Also, we don't apply the same error logging logic to, say, API calls, so I really don't see any good reason to make an exception here.
What we should really remove is these calls (and not introduce new ones), not the comments reminding us to clean up the code by removing them - sooner (better) or later:

Suggested change
console.log("Invalid format on event api: ", message.data);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

API calls that fail are prominently shown in the network tab in red. On failures, they are shown in the regular console.

Incoming messages on websockets are quite hidden under a WS tab in the Networks section and are prone to be overlooked. It is better to make the messages visible in the console if they are not handled than to drop them. The messages are not secret, because still discoverable with the debug interface of any decent browser - but a bit hard to find.

Copy link
Contributor

Choose a reason for hiding this comment

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

Main point is: production code is not for debugging. You should not include any console.log there. Which is why I introduced those TODO comments in the first place. If you want to keep the debugging code in production code: fine, I'm just pointing out (for the last time) that doing so is not a good practice, and they should be removed if we want to have a clean, production-ready code.

src/pages/instances/Events.tsx Show resolved Hide resolved
@edlerd edlerd force-pushed the fix-create-and-start branch 3 times, most recently from 52aa45e to 80b6829 Compare May 30, 2023 15:26
@edlerd edlerd requested a review from lorumic May 30, 2023 15:28
Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

Just one small naming suggestion if you want, apart from that it's ready to go.

src/pages/instances/Events.tsx Outdated Show resolved Hide resolved
@edlerd edlerd merged commit cff83fb into canonical:main Jun 1, 2023
@edlerd edlerd deleted the fix-create-and-start branch June 1, 2023 09:48
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