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.

Sign up for GitHub

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

Jump to bottom

Replace deprecated Lwt usages #21

Merged
merged 10 commits into from
Oct 29, 2019
Merged

Replace deprecated Lwt usages #21

merged 10 commits into from
Oct 29, 2019

Conversation

psafont
Copy link
Member

@psafont psafont commented Sep 5, 2019

Fixes #13 and also prepares the code for Lwt 5.0

lindig
lindig approved these changes Sep 5, 2019
View reviewed changes
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

This looks good.

@psafont
Copy link
Member Author

psafont commented Sep 5, 2019

This needs changes in the daemonization of wsproxy (convert to systemd unit?)

@lindig
Copy link
Contributor

lindig commented Sep 5, 2019

I like that. I would assume that this is not difficult (looking at wsproxy.ml). But who spawns wsproxy currently and how would change that?

@lindig
Copy link
Contributor

lindig commented Sep 5, 2019

wsproxy is spawned from xapi in console.ml using Forkexecd.

@psafont
Copy link
Member Author

psafont commented Sep 5, 2019

My guess is that socket activation needs to be set up along with the service unit.

mseri
mseri reviewed Sep 5, 2019
View reviewed changes
cli/wsproxy.ml
with_fd fd_sock'
~callback:(fun fd ->
let iov = Lwt_unix.io_vector ~buffer ~offset:0 ~length:16384 in
Lwt_unix.recv_msg ~socket:fd ~io_vectors:[iov])
let io_vectors = Lwt_unix.IO_vectors.create () in
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎊

rwdrich
rwdrich reviewed Sep 6, 2019
View reviewed changes
cli/wsproxy.ml Show resolved Hide resolved
psafont added a commit to psafont/xen-api that referenced this pull request Sep 6, 2019
@psafont
console: rely systemd to have wsproxy available
450bef4 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
gaborigloi
gaborigloi reviewed Sep 6, 2019
View reviewed changes
cli/wsproxy.ml Outdated Show resolved Hide resolved
@psafont
CP-32138: replace deprecated Lwt_log with Logs_lwt
f63b94a 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/xen-api that referenced this pull request Sep 9, 2019
@psafont
console: rely systemd to have wsproxy available
ee94306 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/xen-api that referenced this pull request Sep 10, 2019
@psafont
console: rely systemd to have wsproxy available
517e1f2 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/xen-api that referenced this pull request Sep 17, 2019
@psafont
console: rely systemd to have wsproxy available
fa26486 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
gaborigloi
gaborigloi reviewed Sep 18, 2019
View reviewed changes
cli/wsproxy.ml Outdated Show resolved Hide resolved
psafont added a commit to psafont/xen-api that referenced this pull request Sep 30, 2019
@psafont
console: rely systemd to have wsproxy available
57a539e 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@lindig
Copy link
Contributor

lindig commented Oct 17, 2019

What is the state of this? Can we merge this?

@psafont
Copy link
Member Author

psafont commented Oct 17, 2019

The socket/service are not activated when building the packages although they seem to work fine when enabled manually. It needs more changes in the spec files

@psafont
Copy link
Member Author

psafont commented Oct 22, 2019

Testing didn't go as planned: The connection to the guests are not successful when the wsproxy service is up and running, while managed by systemd.

I'll debug the changes in wsproxy as xapi is correctly opening the socket (wsproxy gets run by systemd)

psafont added a commit to psafont/xen-api that referenced this pull request Oct 22, 2019
@psafont
console: rely systemd to have wsproxy available
647c6ba 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
Copy link
Member Author

psafont commented Oct 23, 2019

Comparison of the log format before and after:

Oct 23 12:57:59 localhost wsproxy[26499]: main: About to start connection
Oct 23 12:57:59 localhost wsproxy[26499]: main: Got msg: hybi10:rfb:/var/run/xen/vnc-4
Oct 23 12:57:59 localhost wsproxy[26499]: main: New-style (hybi10) protocol

Oct 23 13:07:27 localhost wsproxy[17923]: wsproxy: [DEBUG] About to start connection
Oct 23 13:07:27 localhost wsproxy[17923]: wsproxy: [DEBUG] Got msg: hybi10:rfb:/var/run/xen/vnc-8
Oct 23 13:07:27 localhost wsproxy[17923]: wsproxy: [WARNING] Malformed msg: not proxying

Processing the message as malformed is not intended, have to look into it. The xapi plumbing looks good so far, however.

@psafont
CP-32138: replace deprecated recv_msg from Lwt_unix
113b572 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
CP-32138: do not daemonize ourselves
9b8d41d 
This will be done by whoever spawns us (e.g. systemd)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
maintenance: whitespace
ae58453 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
CP-32138: simplify path setup
fe655d3 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
CP-32138: Use collaborative log reporter
005e2bb 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
CP-32138: Generate more descriptive messages when match fails
e87cb93 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont psafont marked this pull request as ready for review October 24, 2019 15:32
psafont added a commit to psafont/xen-api that referenced this pull request Oct 24, 2019
@psafont
console: rely systemd to have wsproxy available
742c733 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont psafont mentioned this pull request Oct 24, 2019
Merged
@psafont
cli: stop logging debug-level messages by default
2b9d61a 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
Copy link
Member Author

psafont commented Oct 28, 2019

In order to use socket activation, we cannot create the sockets like we were doing before, we would have to use something like https://github.com/juergenhoetzel/ocaml-systemd

@psafont
CP-32138: Do not manage pid files
47f2c35 
It's not needed anymore due to systemd

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
edwintorok
edwintorok reviewed Oct 28, 2019
View reviewed changes
cli/wsproxy.ml Outdated Show resolved Hide resolved
@psafont
CP-32138: use an already activated socket
a3bbaa0 
Instead of setting up a socket on startup, make the caller
activate the socket and pass it as standard input.

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/xen-api that referenced this pull request Oct 29, 2019
@psafont
console: rely systemd to have wsproxy available
1c530c6 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@edwintorok edwintorok merged commit 4a56fc4 into xapi-project:master Oct 29, 2019
psafont added a commit to psafont/xen-api that referenced this pull request Oct 29, 2019
@psafont
CP-32138: rely systemd to have wsproxy available
4aa7e47 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
BenSimsCitrix pushed a commit to BenSimsCitrix/xen-api that referenced this pull request Mar 26, 2020
@psafont @BenSimsCitrix
CP-32138: rely systemd to have wsproxy available
8183c62 
This change is due to xapi-project/wsproxy#21

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont psafont mentioned this pull request Aug 14, 2020
Merged
@psafont psafont deleted the lwt_dep branch October 22, 2020 12:57
psafont added a commit that referenced this pull request Oct 22, 2020
@psafont
Merge pull request #23 from psafont/modern
b0a85ba 
Pull back changes from #21 (Update Lwt logs and remove daemonization)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwintorok edwintorok edwintorok left review comments

@mseri mseri mseri left review comments

@gaborigloi gaborigloi gaborigloi approved these changes

@lindig lindig lindig approved these changes

@rwdrich rwdrich rwdrich approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move from Lwt_log to the logs library
6 participants
@psafont @lindig @edwintorok @mseri @rwdrich @gaborigloi