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

Fixing the loss of the hostname that is specified in a federated lf f… #1484

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

ChadliaJerad
Copy link
Collaborator

This workaround, based on fed-gen branch, fixes the loss of the RTI hostname in a containerized federated execution.
lfc validator generates a warning though when the domain name is an invalid fully qualified one.
This should not be the case for containerized federated execution, because Docker embeds a DNS server.

Should the validator be fixed accordingly?

…ile. Priority os given to what was given in cmd line though.
@ChadliaJerad ChadliaJerad added bug Something isn't working c Related to C target federated docker Issue related to the docker support labels Nov 21, 2022
@lhstrh
Copy link
Member

lhstrh commented Nov 21, 2022

Yes, I think it's a good idea to update the validator and not have it throw errors if we're in a Docker-based setup. I think we can go ahead and merge this and the validator patch can come through a separate PR.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

@lhstrh lhstrh merged commit 82a33be into fed-gen Nov 21, 2022
Copy link
Collaborator

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

Looks like an improvement. One downside of this approach is that it makes it impossible to override a host specified in the .lf file with "localhost" on the command line. Perhaps it would be better if federationRTIProperties.get("host") defaulted to null? This would have to be checked when it is used to replace null with "localhost".

@ChadliaJerad
Copy link
Collaborator Author

Do you mean that if a host is given in the command line, it will have higher priority, even if it is a localhost?
This makes sense, except that in the context of a federated containerized execution, enforcing an error becomes possible.
Should we default to null and allow overriding whatever is different from localhost only in the federated containerized execution?

@edwardalee
Copy link
Collaborator

I'm not sure I understand, but I think the command line should always override whatever is in the .lf file.

@ChadliaJerad
Copy link
Collaborator Author

A federated containerized program cannot execute if the hostname of the federate (which will be assigned to the RTI) is localhost, because each container will have its own IP address.

Considering a Fed.lf file with:

target C {
    docker: true
}
...
federated reactor at rtiHostName {
    c = new Count();
    p = new Print();
    c.out -> p.in;
}

If we say: lfc --rti localhost src/Fed.lf, the resulting program will not work (after the complete fix of the branch).
This is what is meant by "enforcing an error".

Should we still have the command line always overriding whatever is in the .lf file?

@edwardalee
Copy link
Collaborator

I'm not sure, but we have to also consider uncontainerized applications. The logic I have in mind is simple: The .lf file gives default values. The command line overrides these. It is useful to detect and report errors, but probably we should not just ignore user directives.

@ChadliaJerad
Copy link
Collaborator Author

This PR #1493 makes the command line arguments override those in the .lf file, without defaulting the value of federatedRTIProperties to null, as suggested above.
If we default to null, the changes to the previously implemented logic will break other parts.
The fix in #1493 is therefore safer.
Please check that it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working c Related to C target docker Issue related to the docker support federated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants