-
Notifications
You must be signed in to change notification settings - Fork 6
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
Save Jobs History on Flink #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, EFS is NFS. And NFS is one of those 'you have a problem, you think you will use NFS, and now you have two problems' situations. It plays poorly with a lot of data formats that use any kinda file locking (see https://www.sqlite.org/howtocorrupt.html#_filesystems_with_broken_or_missing_lock_implementations), and the file corruption only shows up in the worst possible times. So I think the primary, and perhaps the only, time to use NFS (and hence EFS) is when providing home directories.
Given we already have the EBS provisioner setup and use it for prometheus, can we not use EBS here too? It does mean that only one pod can write to an EBS volume at a time, but relying on NFS for multiple-replica high availability eventually only leads to tears, pain, blood, stale file handle crashes and death.
Left some inline comments about the kubernetes provider.
Thanks for giving me the deep deets on why EFS/NFS is bad. I was going to use EBS but then I realized something when playing with multiple job managers that made me switch back to EFS:
Does any of that assuage your fears and persuade you one way or the other @yuvipanda? |
doh, so poor: hashicorp/terraform-provider-kubernetes#1775 (comment) maybe I just write a helm config since that works |
YESSS, I always prefer this over raw manifests :) |
Thanks for engaging with me on the EFS issues :) My goal here is not to say 'no EFS ever', but just to make sure we are only using it after we have completely determined that EBS is not an option. So if I understand this correctly, the reason for EFS over EBS are:
I think answers to these questions will help me a lot :) |
No, the reaper process doesn't need to access the EFS mount. It's only checking |
These clowns removed the |
Got confirmation from one of the devs that only the latest two operator versions are supported and one was just released. He's not sure if this documentation applies to the operators as well but it pretty much aligns: https://flink.apache.org/downloads/#update-policy-for-old-releases specific to the operator: https://cwiki.apache.org/confluence/display/FLINK/Release+Schedule+and+Planning |
apiVersion: v1 | ||
kind: PersistentVolume | ||
metadata: | ||
name: flink-historyserver-efs-pv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this has 'historyserver' in the name, so it gets used specifically just for this and not much more :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PersistentVolume is also not namespaced, and should get same treatment as StorageClass
with .Release.Name
. sorry for not catching that earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranchodeluxe I think this still needs to be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working with me on this, @ranchodeluxe. I think using EFS is alright here! I've left some other minor comments, but overall lgtm
Sorry @yuvipanda I thought I muted this by turning it back into a draft so it wouldn't ping you. I'll do that now (it still needs a bit of work) and I'll incorporate your feedback before requesting another review. Here are some answers to some previous questions:
The |
f8b3ea5
to
b47cc34
Compare
requestors changes have been made and I've requested a new review
Mount EFS to Job Managers so they can archive jobs for historical status lookups Addresses #122 Related PR: pangeo-forge/pangeo-forge-cloud-federation#6 Co-authored-by: ranchodeluxe <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for working on this, @ranchodeluxe
apiVersion: v1 | ||
kind: PersistentVolume | ||
metadata: | ||
name: flink-historyserver-efs-pv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PersistentVolume is also not namespaced, and should get same treatment as StorageClass
with .Release.Name
. sorry for not catching that earlier.
terraform/aws/operator.tf
Outdated
"prometheus.io/port" : "9999" | ||
} | ||
}) | ||
value = local_file.flink_operator_config.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this simply be templatefile("flink_operator_config.tpl",{ mount_path=var.historyserver_mount_path })
instead? That way, we can get rid of having to gitignore .yaml
files and save an additional resource here. Also keeps it simpler with one fewer level of redirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why is this a template at all? Can't it be just yamlencode still, with the values for jobmanager.archive.fs.dir
be set to var.historyserver_mount_path
? I think that's much cleaner, and we'll never run into YAML indentation issues due to how templating works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yamlencode
was already creating something that couldn't be parsed by the operator:
[WARN ] Error while trying to split key and value in configuration file /opt/flink/conf/flink-conf.yaml:55: Line is not a key-value pair (missing space after ':'?)
Let me look into this one after I clean other things up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, this inline version of a Map is the only one that doesn't give the parse warning 😆
kubernetes.jobmanager.annotations: {"prometheus.io/scrape": true, "prometheus.io/port": 9999}
I'm gonna keep the template file b/c IMHO yamlencode
is hard to grok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could get that solution to render via templatefile
or yamlencode
correctly so ticked and moving onto better problems for now. Will have a look at it when I get around to getting Prometheus to work better
terraform/aws/helm_historyserver.tf
Outdated
|
||
locals { | ||
# removing lines that start with '#' b/c TF >> helm doesn't like them | ||
filtered_log4j_config = join("\n", [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually really confused about what's happening here. Are we copy pasting the default values from a configmap generated by the operator onto our helm setup, but with #
removed? Why are we copy pasting them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had removed these changes but there were committed to some other branch it seems. It's reusing the default flink-operator-config
now
@yuvipanda gentle nudge with some 🧁 for dessert 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra ping, @ranchodeluxe :)
apiVersion: v1 | ||
kind: PersistentVolume | ||
metadata: | ||
name: flink-historyserver-efs-pv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranchodeluxe I think this still needs to be fixed?
@@ -0,0 +1,4 @@ | |||
efsFileSystemId: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think undoing the 'required' is much cleaner than enforcing them via minLength: 1
. If there is a future change coming in that makes these required, we can modify the schema at that point, no?
Co-authored-by: Yuvi Panda <[email protected]>
Co-authored-by: Yuvi Panda <[email protected]>
Co-authored-by: Yuvi Panda <[email protected]>
Co-authored-by: Yuvi Panda <[email protected]>
alrighty then @yuvipanda, back at this with recent changes so @thodson-usgs can use EFS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks good to me |
Mount EFS to Job Managers so they can archive jobs for historical status lookups
Addresses: pangeo-forge/pangeo-forge-runner#122
Related PR: pangeo-forge/pangeo-forge-runner#131