Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/ood_core/job/account_info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,11 @@ class AccountInfo
# The cluster this account is associated with.
attr_reader :cluster

# The queue this account can use. nil means there is no queue info
# for this account.
attr_reader :queue

def initialize(**opts)
orig_name = opts.fetch(:name, 'unknown')
@name = upcase_accounts? ? orig_name.upcase : orig_name
@qos = opts.fetch(:qos, [])
@cluster = opts.fetch(:cluster, nil)
@queue = opts.fetch(:queue, nil)
end

def to_h
Expand Down
3 changes: 2 additions & 1 deletion lib/ood_core/job/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ def job_name_illegal_chars
ENV["OOD_JOB_NAME_ILLEGAL_CHARS"].to_s
end

# Retrieve the accounts available to use for the current user.
# Retrieve the accounts available to use for the current user.
# The same account might appear muiltiple times if it has access to multiple clusters.
#
# Subclasses that do not implement this will return empty arrays.
# @return [Array<AccountInfo>] the accounts available to the user.
Expand Down
19 changes: 12 additions & 7 deletions lib/ood_core/job/adapters/slurm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,22 +182,27 @@ def get_jobs(id: "", owner: nil, attrs: nil)

def accounts
user = Etc.getlogin
args = ['-nP', 'show', 'users', 'withassoc', 'format=account,cluster,partition,qos', 'where', "user=#{user}"]
args = ['-nP', 'show', 'users', 'withassoc', 'format=account,cluster,qos', 'where', "user=#{user}"]

[].tap do |accts|
[].tap do |associations|
call('sacctmgr', *args).each_line do |line|
acct, cluster, queue, qos = line.split('|')
acct, cluster, qos = line.split('|')
next if acct.nil? || acct.chomp.empty?

args = {
associations << {
name: acct,
qos: qos.to_s.chomp.split(','),
cluster: cluster,
queue: queue.to_s.empty? ? nil : queue
}
info = OodCore::Job::AccountInfo.new(**args) unless acct.nil?
accts << info unless acct.nil?
end
end.group_by do |x|
[x[:name], x[:cluster]]
end.map do |(name, cluster), assocs|
OodCore::Job::AccountInfo.new(
name: name,
cluster: cluster,
qos: (assocs.flat_map { |x| x[:qos] }).uniq,
)
end
end

Expand Down
49 changes: 22 additions & 27 deletions spec/fixtures/output/slurm/sacctmgr_show_accts.txt
Original file line number Diff line number Diff line change
@@ -1,27 +1,22 @@
pzs0715|ascend|partition_a|ascend-default
pzs0714|ascend|partition_b|ascend-default
pzs1124|owens||owens-default,staff,phoenix,geophys,hal,gpt
pzs1118|owens||owens-default
pzs1117|owens||owens-default
pzs1010|owens||owens-default
pzs0715|owens||owens-default
pzs0714|owens||owens-default
pde0006|owens||owens-default
pas2051|owens||owens-default
pas1871|owens||owens-default
pas1754|owens||owens-default
pas1604|owens||owens-default
pzs1124|pitzer||pitzer-default
pzs1118|pitzer||pitzer-default
pzs1117|pitzer||pitzer-default
pzs1010|pitzer||pitzer-default
pzs0715|pitzer||pitzer-default
pzs0714|pitzer||pitzer-default
pde0006|pitzer||pitzer-default
pas2051|pitzer||pitzer-default
pas1871|pitzer||pitzer-default
pas1754|pitzer||pitzer-default
pas1604|pitzer||pitzer-default



pzs1124|owens|owens-default,staff,phoenix,geophys,hal,gpt
pzs1118|owens|owens-default
pzs1117|owens|owens-default
pzs1010|owens|owens-default
pzs0715|owens|owens-default
pzs0714|owens|owens-default
pde0006|owens|owens-default
pas2051|owens|owens-default
pas1871|owens|owens-default
pas1754|owens|owens-default
pas1604|owens|owens-default
pzs1124|pitzer|pitzer-default
pzs1118|pitzer|pitzer-default
pzs1117|pitzer|pitzer-default
pzs1010|pitzer|pitzer-default
pzs0715|pitzer|pitzer-default
pzs0714|pitzer|pitzer-default
pde0006|pitzer|pitzer-default
pas2051|pitzer|pitzer-default
pas1871|pitzer|pitzer-default
pas1754|pitzer|pitzer-default
pas1604|pitzer|pitzer-default
28 changes: 6 additions & 22 deletions spec/job/adapters/slurm_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1305,17 +1305,17 @@ def job_info(opts = {})
it 'returns the correct accounts names' do
allow(Etc).to receive(:getlogin).and_return('me')
allow(Open3).to receive(:capture3)
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,partition,qos', 'where', 'user=me', {stdin_data: ''})
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,qos', 'where', 'user=me', {stdin_data: ''})
.and_return([File.read('spec/fixtures/output/slurm/sacctmgr_show_accts.txt'), '', double("success?" => true)])

expect(subject.accounts.map(&:to_s).uniq).to eq(expected_accounts)
expect(subject.accounts.map(&:to_s).uniq.to_set).to eq(expected_accounts.to_set)
end

# TODO test for qos & cluster once the API solidifies
it 'parses qos correctly' do
allow(Etc).to receive(:getlogin).and_return('me')
allow(Open3).to receive(:capture3)
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,partition,qos', 'where', 'user=me', {stdin_data: ''})
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,qos', 'where', 'user=me', {stdin_data: ''})
.and_return([File.read('spec/fixtures/output/slurm/sacctmgr_show_accts.txt'), '', double("success?" => true)])

accts = subject.accounts
Expand All @@ -1327,22 +1327,6 @@ def job_info(opts = {})
expect(acct.qos).to eq(["#{acct.cluster}-default"])
end
end

it 'parses partition correctly' do
allow(Etc).to receive(:getlogin).and_return('me')
allow(Open3).to receive(:capture3)
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,partition,qos', 'where', 'user=me', {stdin_data: ''})
.and_return([File.read('spec/fixtures/output/slurm/sacctmgr_show_accts.txt'), '', double("success?" => true)])

accts = subject.accounts
acct_w_partitions = accts.select { |a| a.cluster == 'ascend' }
acct_w_no_partitions = accts.select { |a| a.queue.nil? }

expect(acct_w_partitions.size).to eq(2)
expect(accts - acct_w_no_partitions).to eq(acct_w_partitions)
expect(acct_w_partitions.select {|a| a.name == 'pzs0715'}.first.queue).to eq('partition_a')
expect(acct_w_partitions.select {|a| a.name == 'pzs0714'}.first.queue).to eq('partition_b')
end
end

context 'when sacctmgr fails' do
Expand All @@ -1351,7 +1335,7 @@ def job_info(opts = {})
it 'raises the error' do
allow(Etc).to receive(:getlogin).and_return('me')
allow(Open3).to receive(:capture3)
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,partition,qos', 'where', 'user=me', {stdin_data: ''})
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,qos', 'where', 'user=me', {stdin_data: ''})
.and_return(['', 'the error message', double("success?" => false)])

expect { subject.accounts }.to raise_error(OodCore::Job::Adapters::Slurm::Batch::Error, 'the error message')
Expand All @@ -1365,11 +1349,11 @@ def job_info(opts = {})
it 'returns the correct accounts' do
allow(Etc).to receive(:getlogin).and_return('me')
allow(Open3).to receive(:capture3)
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,partition,qos', 'where', 'user=me', {stdin_data: ''})
.with({}, 'sacctmgr', '-nP', 'show', 'users', 'withassoc', 'format=account,cluster,qos', 'where', 'user=me', {stdin_data: ''})
.and_return([File.read('spec/fixtures/output/slurm/sacctmgr_show_accts.txt'), '', double("success?" => true)])

with_modified_env({ OOD_UPCASE_ACCOUNTS: 'true'}) do
expect(subject.accounts.map(&:to_s).uniq).to eq(expected_accounts)
expect(subject.accounts.map(&:to_s).uniq.to_set).to eq(expected_accounts.to_set)
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, now that this can return duplicates I think we shouldn't use to_set and in fact ensure that duplicates are returned as that's.

It's possible (probable/) that I should not have started with uniq, but with no second pair of eyes, it slipped through.

In any case, if returning duplicates is expected out of this API, then we should ensure that we test for that duplication. Maybe we need to test against tuples with 2 keys account & cluster pairs?

end
end
end
Expand Down